Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add proposed API to specific workspace-specific environment variables via EnvironmentVariableCollection #179323

Merged
merged 68 commits into from
Apr 20, 2023
Merged

Add proposed API to specific workspace-specific environment variables via EnvironmentVariableCollection #179323

merged 68 commits into from
Apr 20, 2023

Conversation

karrtikr
Copy link
Contributor

@karrtikr karrtikr commented Apr 5, 2023 •
edited
Loading

For #171173

@karrtikr

This comment was marked as resolved.

Sign in to view
@karrtikr karrtikr changed the title Add proposed API to specify workspace-specific EnvironmentVariableCollection mutators Add proposed API to specific workspace-specific environment variables via EnvironmentVariableCollection Apr 12, 2023
meganrogge
meganrogge reviewed Apr 20, 2023
View reviewed changes
src/vs/platform/terminal/common/terminal.ts
@@ -600,6 +601,7 @@ export interface ITerminalProcessOptions {
};
windowsEnableConpty: boolean;
environmentVariableCollections: ISerializableEnvironmentVariableCollections | undefined;
workspaceFolder: IWorkspaceFolder | undefined;
Copy link
Contributor

@meganrogge meganrogge Apr 20, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
workspaceFolder: IWorkspaceFolder | undefined;
workspaceFolder?: IWorkspaceFolder;

then you can remove all of the environmentVariableCollections: undefined

from

vscode/src/vs/platform/terminal/test/node/terminalEnvironment.test.ts

Lines 13 to 16 in 0dc2f48

const enabledProcessOptions: ITerminalProcessOptions = { shellIntegration: { enabled: true, suggestEnabled: false }, windowsEnableConpty: true, environmentVariableCollections: undefined, workspaceFolder: undefined };
const disabledProcessOptions: ITerminalProcessOptions = { shellIntegration: { enabled: false, suggestEnabled: false }, windowsEnableConpty: true, environmentVariableCollections: undefined, workspaceFolder: undefined };
const winptyProcessOptions: ITerminalProcessOptions = { shellIntegration: { enabled: true, suggestEnabled: false }, windowsEnableConpty: false, environmentVariableCollections: undefined, workspaceFolder: undefined };

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should not miss to pass workspaceFolder when using ITerminalProcessOptions, so I don't think making it optional for sake of tests is the best approach.

karrtikr
karrtikr commented Apr 20, 2023
View reviewed changes
Copy link
Contributor Author

@karrtikr karrtikr left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@meganrogge Thanks for the review! I've cleaned up the bits pointed out. Please have a look.

@karrtikr karrtikr requested a review from meganrogge April 20, 2023 18:44
karrtikr
karrtikr commented Apr 20, 2023
View reviewed changes
src/vs/workbench/contrib/terminal/common/terminalStorageKeys.ts
@@ -8,7 +8,8 @@ export const enum TerminalStorageKeys {
SuggestedRendererType = 'terminal.integrated.suggestedRendererType',
TabsListWidthHorizontal = 'tabs-list-width-horizontal',
TabsListWidthVertical = 'tabs-list-width-vertical',
EnvironmentVariableCollections = 'terminal.integrated.environmentVariableCollections',
DeprecatedEnvironmentVariableCollections = 'terminal.integrated.environmentVariableCollections',
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note older storage for env collection is deprecated now and will be cleared.

@meganrogge
Copy link
Contributor

@karrtikr some test failures but o/w LGTM

meganrogge
meganrogge previously approved these changes Apr 20, 2023
View reviewed changes
@karrtikr
Copy link
Contributor Author

Reverting fix for remote terminals which is leading to smoke tests timing out, will be tackled separately.

@karrtikr karrtikr merged commit f80485d into main Apr 20, 2023
@karrtikr karrtikr deleted the kartik/terminal branch April 20, 2023 22:00
@karrtikr karrtikr mentioned this pull request Apr 21, 2023
Open
@github-actions github-actions bot locked and limited conversation to collaborators Jun 4, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@Tyriar Tyriar Tyriar left review comments

@eleanorjboyd eleanorjboyd eleanorjboyd approved these changes

@meganrogge meganrogge meganrogge left review comments

Assignees

@karrtikr karrtikr

Labels
None yet
Projects
None yet
Milestone
April 2023
Development

Successfully merging this pull request may close these issues.
5 participants
@karrtikr @meganrogge @Tyriar @eleanorjboyd @vscodenpa