Allow specifying workspace-specific EnvironmentVariableCollection mutators

[Tyriar]

For example:

export interface EnvironmentVariableMutator {
	readonly type: EnvironmentVariableMutatorType;
	readonly value: string;
	readonly workspace: Workspace | undefined;
}

export interface EnvironmentVariableCollection extends Iterable<[variable: string, mutator: EnvironmentVariableMutator]> {
	replace(variable: string, value: string, workspace?: Workspace): void;
	append(variable: string, value: string, workspace?: Workspace): void;
	prepend(variable: string, value: string, workspace?: Workspace): void;
}

Context: microsoft/vscode-python#20492 (comment)

[karrtikr]

Feedback from API sync:

As we anticipate we might need shell scoping as well in the future, use something similar to

workspace.getConfiguration(section?: string, scope?: ConfigurationScope): WorkspaceConfiguration;

instead.

Updated proposal:

export interface EnvironmentVariableMutator {
	readonly type: EnvironmentVariableMutatorType;
	readonly value: string;
	readonly scope: EnvironmentVariableScope | undefined;
}

export interface EnvironmentVariableCollection extends Iterable<[variable: string, mutator: EnvironmentVariableMutator]> {
	replace(variable: string, value: string, scope?: EnvironmentVariableScope): void;
	append(variable: string, value: string, scope?: EnvironmentVariableScope): void;
	prepend(variable: string, value: string, scope?: EnvironmentVariableScope): void;
}

export type EnvironmentVariableScope = {
    /**
     * The workspace folder to which this environment variable collection applies to.
     * If unspecified, the collection applies to all workspace folders.
     */
    workspaceFolder?: WorkspaceFolder;
};

One issue with this is that readonly scope: EnvironmentVariableScope | undefined; carries optional properties. Hence an alternative could be:

export interface EnvironmentVariableMutator {
	readonly type: EnvironmentVariableMutatorType;
	readonly value: string;
	readonly scope: Required<EnvironmentVariableScope> | undefined;
}

[Tyriar]

One issue with this is that readonly scope: EnvironmentVariableScope | undefined; carries optional properties. Hence an alternative could be:

The required would need to be removed when we added a second one anyway, we can just treat {} and { workspaceFolder: undefined } the same as undefined

[karrtikr]

Sorry what do you mean by the "second one"? Mostly I was trying to follow Extension-API-guidelines:

For readonly properties on interfaces that VS Code exposes to extensions: Use | undefined as this makes it clear the property exists but has the value undefined.

But I guess we don't have to strictly follow it?

[Tyriar]

@karrtikr by a second one I mean a second scope, we don't want the user to explicitly enumerate all the scopes to set it which Required<> would do.

[karrtikr]

Updated proposal:

export interface EnvironmentVariableMutator {
	readonly type: EnvironmentVariableMutatorType;
	readonly value: string;
	readonly scope: EnvironmentVariableScope | undefined;
}

export interface EnvironmentVariableCollection extends Iterable<[variable: string, mutator: EnvironmentVariableMutator]> {
	replace(variable: string, value: string, scope?: EnvironmentVariableScope): void;
	append(variable: string, value: string, scope?: EnvironmentVariableScope): void;
	prepend(variable: string, value: string, scope?: EnvironmentVariableScope): void;
	get(variable: string, scope?: EnvironmentVariableScope): EnvironmentVariableMutator | undefined;
	delete(variable: string, scope?: EnvironmentVariableScope): void;
	clear(scope?: EnvironmentVariableScope): void;
}

export type EnvironmentVariableScope = {
	/**
	* The workspace folder to which this collection applies to. If unspecified, collection applies to all workspace folders.
	*/
	workspaceFolder?: WorkspaceFolder;
};

[karrtikr]

Based on discussion in API sync, we should probably deprecate existing environmentVariableCollection API and merge it with the scoped one (similar to workspace.getConfiguration())

export interface ExtensionContext {
	/**
	 * Gets the extension's environment variable collection for this workspace, enabling changes
	 * to be applied to terminal environment variables.
	 *
	 * @deprecated Use {@link getEnvironmentVariableCollection} instead.
	 */
	readonly environmentVariableCollection: EnvironmentVariableCollection;
	/**
	 * Gets the extension's environment variable collection for this workspace, enabling changes
	 * to be applied to terminal environment variables.
	 *
	 * @param scope The scope to which the environment variable collection applies to.
	 */
	getEnvironmentVariableCollection(scope?: EnvironmentVariableScope): EnvironmentVariableCollection;
}

export type EnvironmentVariableScope = {
	/**
	* Any specific workspace folder to get collection for. If unspecified, collection applicable to all workspace folders is returned.
	*/
	workspaceFolder?: WorkspaceFolder;
};

[Tyriar]

Assigning myself too so it's easier for me to track 🙂

[karrtikr]

@meganrogge @Tyriar When viewing contributions, is it possible to open the preview of the markdown directly instead of the .md file?

as opposed to

I can open preview using:

[Tyriar]

@karrtikr since this is an advanced feature I think the raw markdown is more flexible as the user can navigate and copy section of it better. So I don't think this is a necessary change personally

[Tyriar]

Feedback before finalization:

• Add an interface GlobalEnviornmentVariableCollection implements EnvironmentVariableCollection
• Add getScoped to the GlobalEnviornmentVariableCollection
• Clarify how scoped interacts with the non-scoped collection
• Don't deprecate

[karrtikr]

Updated proposal:

export interface ExtensionContext {
    /**
     * Gets the extension's global environment variable collection for this workspace, enabling changes to be
     * applied to terminal environment variables.
     */
    readonly environmentVariableCollection: GlobalEnvironmentVariableCollection;
}

interface GlobalEnvironmentVariableCollection extends EnvironmentVariableCollection {
    /**
     * Gets scope-specific environment variable collection for the extension. This enables alterations to
     * terminal environment variables solely within the designated scope, and is applied in addition to (and
     * after) the global collection.
     *
     * Each object obtained through this method is isolated and does not impact objects for other scopes,
     * including the global collection.
     *
     * @param scope The scope to which the environment variable collection applies to.
     */
    getScoped(scope: EnvironmentVariableScope): EnvironmentVariableCollection;
}

export type EnvironmentVariableScope = {
    /**
     * Any specific workspace folder to get collection for. If unspecified, collection applicable to all workspace folders is returned.
     */
    workspaceFolder?: WorkspaceFolder;
};

[Tyriar]

Each object obtained through this method is isolated and does not impact objects for other scopes, including the global collection.

Should we also call out how it's applied in addition to (and after) the global collection?

Looks good otherwise 👍

[karrtikr]

Sure, I have modified the docstrings to append it

I plan to have the implementation ready but only merge and adopt it in Python extension next week to avoid multiple iterations.