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.

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

EnvironmentVariableMutatorOptions API proposal #182883

Merged
merged 16 commits into from
May 23, 2023
Merged

EnvironmentVariableMutatorOptions API proposal #182883

merged 16 commits into from
May 23, 2023

Conversation

Tyriar
Copy link
Member

@Tyriar Tyriar commented May 18, 2023

Part of #179476

@Tyriar Tyriar added this to the May 2023 milestone May 18, 2023
@Tyriar Tyriar self-assigned this May 18, 2023
@Tyriar
Copy link
Member Author

Tyriar commented May 19, 2023

When setting applyInShellIntegration in the terminal-sample:

image
image

PATH:

image

@Tyriar Tyriar requested a review from karrtikr May 19, 2023 14:28
@Tyriar Tyriar marked this pull request as ready for review May 19, 2023 14:28
@Tyriar Tyriar changed the title Initial impl of env var collection options EnvironmentVariableMutatorOptions API proposal May 19, 2023
@Tyriar Tyriar removed the request for review from karrtikr May 23, 2023 13:47
sandy081
sandy081 previously approved these changes May 23, 2023
src/vs/workbench/api/browser/mainThreadTerminalService.ts Outdated Show resolved Hide resolved
sandy081
sandy081 previously approved these changes May 23, 2023
sandy081
sandy081 previously approved these changes May 23, 2023
karrtikr
karrtikr previously approved these changes May 23, 2023
@Tyriar Tyriar dismissed stale reviews from karrtikr and sandy081 via 438eb68 May 23, 2023 16:02
Comment on lines +14 to +19
/**
* Apply to the environment just before the process is created.
*
* Defaults to true.
*/
applyAtProcessCreation?: boolean;
Copy link
Contributor

@karrtikr karrtikr May 23, 2023

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

Let's bring this up at the API sync, undefined not being true would be harmful here imo.

@Tyriar Tyriar merged commit f043f49 into main May 23, 2023
@Tyriar Tyriar deleted the tyriar/179476 branch May 23, 2023 16:33
this._setIfDiffers(variable, { value, type: EnvironmentVariableMutatorType.Replace, scope });
replace(variable: string, value: string, options: vscode.EnvironmentVariableMutatorOptions | undefined, scope: vscode.EnvironmentVariableScope | undefined): void {
if (this._extension && options) {
isProposedApiEnabled(this._extension, 'envCollectionOptions');
Copy link
Contributor

@karrtikr karrtikr May 23, 2023

Choose a reason for hiding this comment

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

@Tyriar This seems to return a boolean but isn't consumed anywhere, did we mean to use checkProposedApiEnabled instead? Same for other places it's used.

Copy link
Contributor

Choose a reason for hiding this comment

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

I've changed it in #183255.

@github-actions github-actions bot locked and limited conversation to collaborators Jul 7, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants