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
Show file tree
Hide file tree
Changes from all commits
Commits
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
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
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
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
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

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