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

EnvironmentVariableMutatorOptions API proposal #182883

Merged
merged 16 commits into from
May 23, 2023
Merged

EnvironmentVariableMutatorOptions API proposal #182883

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
689c912
Initial impl of env var collection options
Tyriar May 18, 2023
d6b2cff
Merge remote-tracking branch 'origin/main' into tyriar/179476
Tyriar May 18, 2023
3ad9552
Merge branch 'main' into tyriar/179476
Tyriar May 19, 2023
f3fe4f9
Fix options in extension API pass through
Tyriar May 19, 2023
456dd5b
Fix considering options in merged collection
Tyriar May 19, 2023
2830bd2
Remove test logs
Tyriar May 19, 2023
cd5fe46
Add proposed check to options
Tyriar May 19, 2023
c5aa6c1
Throw when invalid options are used
Tyriar May 19, 2023
8e9b5a2
Fix throw condition
Tyriar May 19, 2023
0192657
Add missing comment to EnvironmentVariableMutatorOptions
Tyriar May 19, 2023
16533f5
Merge branch 'main' into tyriar/179476
Tyriar May 23, 2023
2fe1a30
Fix test failures
Tyriar May 23, 2023
35bdb61
Add mutator options to some tests
Tyriar May 23, 2023
8b079fa
Remove unwanted log
Tyriar May 23, 2023
27293f1
Fix env var collection api tests
Tyriar May 23, 2023
438eb68
Actually fix the tests, options is an empty object not undefined
Tyriar May 23, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 6 additions & 6 deletions extensions/vscode-api-tests/src/singlefolder-tests/terminal.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -850,17 +850,17 @@ import { assertNoRpc, poll } from '../utils';
collection.prepend('C', '~c2~');

// Verify get
deepStrictEqual(collection.get('A'), { value: '~a2~', type: EnvironmentVariableMutatorType.Replace, scope: undefined });
deepStrictEqual(collection.get('B'), { value: '~b2~', type: EnvironmentVariableMutatorType.Append, scope: undefined });
deepStrictEqual(collection.get('C'), { value: '~c2~', type: EnvironmentVariableMutatorType.Prepend, scope: undefined });
deepStrictEqual(collection.get('A'), { value: '~a2~', type: EnvironmentVariableMutatorType.Replace, scope: undefined, options: {} });
deepStrictEqual(collection.get('B'), { value: '~b2~', type: EnvironmentVariableMutatorType.Append, scope: undefined, options: {} });
deepStrictEqual(collection.get('C'), { value: '~c2~', type: EnvironmentVariableMutatorType.Prepend, scope: undefined, options: {} });

// Verify forEach
const entries: [string, EnvironmentVariableMutator][] = [];
collection.forEach((v, m) => entries.push([v, m]));
deepStrictEqual(entries, [
['A', { value: '~a2~', type: EnvironmentVariableMutatorType.Replace, scope: undefined }],
['B', { value: '~b2~', type: EnvironmentVariableMutatorType.Append, scope: undefined }],
['C', { value: '~c2~', type: EnvironmentVariableMutatorType.Prepend, scope: undefined }]
['A', { value: '~a2~', type: EnvironmentVariableMutatorType.Replace, scope: undefined, options: {} }],
['B', { value: '~b2~', type: EnvironmentVariableMutatorType.Append, scope: undefined, options: {} }],
['C', { value: '~c2~', type: EnvironmentVariableMutatorType.Prepend, scope: undefined, options: {} }]
]);
});
});
Expand Down
12 changes: 6 additions & 6 deletions src/vs/platform/terminal/common/environmentVariable.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,24 +11,24 @@ export enum EnvironmentVariableMutatorType {
Append = 2,
Prepend = 3
}
// export enum EnvironmentVariableMutatorTiming {
// AtSpawn = 1,
// AfterShellIntegration = 2
// // TODO: Do we need a both?
// }
export interface IEnvironmentVariableMutator {
readonly variable: string;
readonly value: string;
readonly type: EnvironmentVariableMutatorType;
readonly scope?: EnvironmentVariableScope;
// readonly timing?: EnvironmentVariableMutatorTiming;
readonly options?: IEnvironmentVariableMutatorOptions;
}

export interface IEnvironmentDescriptionMutator {
readonly description: string | undefined;
readonly scope?: EnvironmentVariableScope;
}

export interface IEnvironmentVariableMutatorOptions {
applyAtProcessCreation?: boolean;
applyAtShellIntegration?: boolean;
}

export type EnvironmentVariableScope = {
workspaceFolder?: IWorkspaceFolderData;
};
Expand Down
50 changes: 29 additions & 21 deletions src/vs/platform/terminal/common/environmentVariableCollection.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,11 +8,11 @@ import { EnvironmentVariableMutatorType, EnvironmentVariableScope, IEnvironmentV

type VariableResolver = (str: string) => Promise<string>;

// const mutatorTypeToLabelMap: Map<EnvironmentVariableMutatorType, string> = new Map([
// [EnvironmentVariableMutatorType.Append, 'APPEND'],
// [EnvironmentVariableMutatorType.Prepend, 'PREPEND'],
// [EnvironmentVariableMutatorType.Replace, 'REPLACE']
// ]);
const mutatorTypeToLabelMap: Map<EnvironmentVariableMutatorType, string> = new Map([
[EnvironmentVariableMutatorType.Append, 'APPEND'],
[EnvironmentVariableMutatorType.Prepend, 'PREPEND'],
[EnvironmentVariableMutatorType.Replace, 'REPLACE']
]);

export class MergedEnvironmentVariableCollection implements IMergedEnvironmentVariableCollection {
private readonly map: Map<string, IExtensionOwnedEnvironmentVariableMutator[]> = new Map();
Expand Down Expand Up @@ -46,7 +46,8 @@ export class MergedEnvironmentVariableCollection implements IMergedEnvironmentVa
value: mutator.value,
type: mutator.type,
scope: mutator.scope,
variable: mutator.variable
variable: mutator.variable,
options: mutator.options
};
if (!extensionMutator.scope) {
delete extensionMutator.scope; // Convenient for tests
Expand All @@ -69,26 +70,33 @@ export class MergedEnvironmentVariableCollection implements IMergedEnvironmentVa
const actualVariable = isWindows ? lowerToActualVariableNames![variable.toLowerCase()] || variable : variable;
for (const mutator of mutators) {
const value = variableResolver ? await variableResolver(mutator.value) : mutator.value;
// if (mutator.timing === EnvironmentVariableMutatorTiming.AfterShellIntegration) {
// const key = `VSCODE_ENV_${mutatorTypeToLabelMap.get(mutator.type)!}`;
// env[key] = (env[key] ? env[key] + ':' : '') + variable + '=' + value;
// continue;
// }
switch (mutator.type) {
case EnvironmentVariableMutatorType.Append:
env[actualVariable] = (env[actualVariable] || '') + value;
break;
case EnvironmentVariableMutatorType.Prepend:
env[actualVariable] = value + (env[actualVariable] || '');
break;
case EnvironmentVariableMutatorType.Replace:
env[actualVariable] = value;
break;
// Default: true
if (mutator.options?.applyAtProcessCreation ?? true) {
switch (mutator.type) {
case EnvironmentVariableMutatorType.Append:
env[actualVariable] = (env[actualVariable] || '') + value;
break;
case EnvironmentVariableMutatorType.Prepend:
env[actualVariable] = value + (env[actualVariable] || '');
break;
case EnvironmentVariableMutatorType.Replace:
env[actualVariable] = value;
break;
}
}
// Default: false
if (mutator.options?.applyAtShellIntegration ?? false) {
const key = `VSCODE_ENV_${mutatorTypeToLabelMap.get(mutator.type)!}`;
env[key] = (env[key] ? env[key] + ':' : '') + variable + '=' + this._encodeColons(value);
}
}
}
}

private _encodeColons(value: string): string {
return value.replaceAll(':', '\\x3a');
}

diff(other: IMergedEnvironmentVariableCollection, scope: EnvironmentVariableScope | undefined): IMergedEnvironmentVariableCollectionDiff | undefined {
const added: Map<string, IExtensionOwnedEnvironmentVariableMutator[]> = new Map();
const changed: Map<string, IExtensionOwnedEnvironmentVariableMutator[]> = new Map();
Expand Down
56 changes: 38 additions & 18 deletions src/vs/workbench/api/common/extHostTerminalService.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ import { withNullAsUndefined } from 'vs/base/common/types';
import { Promises } from 'vs/base/common/async';
import { EditorGroupColumn } from 'vs/workbench/services/editor/common/editorGroupColumn';
import { ViewColumn } from 'vs/workbench/api/common/extHostTypeConverters';
import { isProposedApiEnabled } from 'vs/workbench/services/extensions/common/extensions';

export interface IExtHostTerminalService extends ExtHostTerminalServiceShape, IDisposable {

Expand Down Expand Up @@ -826,7 +827,7 @@ export abstract class BaseExtHostTerminalService extends Disposable implements I
public getEnvironmentVariableCollection(extension: IExtensionDescription): IEnvironmentVariableCollection {
let collection = this._environmentVariableCollections.get(extension.identifier.value);
if (!collection) {
collection = new EnvironmentVariableCollection();
collection = new EnvironmentVariableCollection(extension);
this._setEnvironmentVariableCollection(extension.identifier.value, collection);
}
return collection.getScopedEnvironmentVariableCollection(undefined);
Expand All @@ -841,7 +842,7 @@ export abstract class BaseExtHostTerminalService extends Disposable implements I
public $initEnvironmentVariableCollections(collections: [string, ISerializableEnvironmentVariableCollection][]): void {
collections.forEach(entry => {
const extensionIdentifier = entry[0];
const collection = new EnvironmentVariableCollection(entry[1]);
const collection = new EnvironmentVariableCollection(undefined, entry[1]);
this._setEnvironmentVariableCollection(extensionIdentifier, collection);
});
}
Expand Down Expand Up @@ -883,6 +884,11 @@ class EnvironmentVariableCollection {
get onDidChangeCollection(): Event<void> { return this._onDidChangeCollection && this._onDidChangeCollection.event; }

constructor(
// HACK: Only check proposed options if extension is set (when the collection is not
// restored by serialization). This saves us from getting the extension details and
// shouldn't ever happen since you can only set them initially via the proposed check.
// TODO: This should be removed when the env var extension API(s) are stabilized
private readonly _extension: IExtensionDescription | undefined,
serialized?: ISerializableEnvironmentVariableCollection
) {
this.map = new Map(serialized);
Expand All @@ -899,19 +905,31 @@ class EnvironmentVariableCollection {
return scopedCollection;
}

replace(variable: string, value: string, scope: vscode.EnvironmentVariableScope | undefined): void {
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 •
edited
Loading

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.

}
this._setIfDiffers(variable, { value, type: EnvironmentVariableMutatorType.Replace, options: options ?? {}, scope });
}

append(variable: string, value: string, scope: vscode.EnvironmentVariableScope | undefined): void {
this._setIfDiffers(variable, { value, type: EnvironmentVariableMutatorType.Append, scope });
append(variable: string, value: string, options: vscode.EnvironmentVariableMutatorOptions | undefined, scope: vscode.EnvironmentVariableScope | undefined): void {
if (this._extension && options) {
isProposedApiEnabled(this._extension, 'envCollectionOptions');
}
this._setIfDiffers(variable, { value, type: EnvironmentVariableMutatorType.Append, options: options ?? {}, scope });
}

prepend(variable: string, value: string, scope: vscode.EnvironmentVariableScope | undefined): void {
this._setIfDiffers(variable, { value, type: EnvironmentVariableMutatorType.Prepend, scope });
prepend(variable: string, value: string, options: vscode.EnvironmentVariableMutatorOptions | undefined, scope: vscode.EnvironmentVariableScope | undefined): void {
if (this._extension && options) {
isProposedApiEnabled(this._extension, 'envCollectionOptions');
}
this._setIfDiffers(variable, { value, type: EnvironmentVariableMutatorType.Prepend, options: options ?? {}, scope });
}

private _setIfDiffers(variable: string, mutator: vscode.EnvironmentVariableMutator & { scope: vscode.EnvironmentVariableScope | undefined }): void {
if (mutator.options && mutator.options.applyAtProcessCreation === false && !mutator.options.applyAtShellIntegration) {
throw new Error('EnvironmentVariableMutatorOptions must apply at either process creation or shell integration');
}
if (!mutator.scope) {
delete (mutator as any).scope; // Convenient for tests
}
Expand All @@ -928,6 +946,7 @@ class EnvironmentVariableCollection {
get(variable: string, scope: vscode.EnvironmentVariableScope | undefined): vscode.EnvironmentVariableMutator | undefined {
const key = this.getKey(variable, scope);
const value = this.map.get(key);
// TODO: Set options to defaults if needed
return value ? convertMutator(value) : undefined;
}

Expand All @@ -944,11 +963,11 @@ class EnvironmentVariableCollection {
return workspaceFolder ? workspaceFolder.uri.toString() : undefined;
}

public getVariableMap(scope: vscode.EnvironmentVariableScope | undefined): Map<string, IEnvironmentVariableMutator> {
const map = new Map<string, IEnvironmentVariableMutator>();
public getVariableMap(scope: vscode.EnvironmentVariableScope | undefined): Map<string, vscode.EnvironmentVariableMutator> {
const map = new Map<string, vscode.EnvironmentVariableMutator>();
for (const [key, value] of this.map) {
if (this.getScopeKey(value.scope) === this.getScopeKey(scope)) {
map.set(key, value);
map.set(key, convertMutator(value));
}
}
return map;
Expand Down Expand Up @@ -1022,24 +1041,24 @@ class ScopedEnvironmentVariableCollection implements vscode.EnvironmentVariableC
return this.collection.getScopedEnvironmentVariableCollection(this.scope);
}

replace(variable: string, value: string): void {
this.collection.replace(variable, value, this.scope);
replace(variable: string, value: string, options?: vscode.EnvironmentVariableMutatorOptions | undefined): void {
this.collection.replace(variable, value, options, this.scope);
}

append(variable: string, value: string): void {
this.collection.append(variable, value, this.scope);
append(variable: string, value: string, options?: vscode.EnvironmentVariableMutatorOptions | undefined): void {
this.collection.append(variable, value, options, this.scope);
}

prepend(variable: string, value: string): void {
this.collection.prepend(variable, value, this.scope);
prepend(variable: string, value: string, options?: vscode.EnvironmentVariableMutatorOptions | undefined): void {
this.collection.prepend(variable, value, options, this.scope);
}

get(variable: string): vscode.EnvironmentVariableMutator | undefined {
return this.collection.get(variable, this.scope);
}

forEach(callback: (variable: string, mutator: vscode.EnvironmentVariableMutator, collection: vscode.EnvironmentVariableCollection) => any, thisArg?: any): void {
this.collection.getVariableMap(this.scope).forEach((value, variable) => callback.call(thisArg, variable, convertMutator(value), this), this.scope);
this.collection.getVariableMap(this.scope).forEach((value, variable) => callback.call(thisArg, variable, value, this), this.scope);
}

[Symbol.iterator](): IterableIterator<[variable: string, mutator: vscode.EnvironmentVariableMutator]> {
Expand Down Expand Up @@ -1102,6 +1121,7 @@ function asTerminalColor(color?: vscode.ThemeColor): ThemeColor | undefined {
function convertMutator(mutator: IEnvironmentVariableMutator): vscode.EnvironmentVariableMutator {
const newMutator = { ...mutator };
newMutator.scope = newMutator.scope ?? undefined;
newMutator.options = newMutator.options ?? undefined;
delete (newMutator as any).variable;
return newMutator as vscode.EnvironmentVariableMutator;
}
Loading