Updates to environment collection workspace API #182238
Conversation
| return { | ||
| ...collection.getScopedEnvironmentVariableCollection(undefined), getScopedEnvironmentVariableCollection(scope) { | ||
| return collection!.getScopedEnvironmentVariableCollection(scope); | ||
| }, | ||
| }; |
There was a problem hiding this comment.
I think it would be simpler if you kept EnvironmentVariableCollection (or DefaultEnvironmentVariableCollection) and ScopedEnvironmentVariableCollection separate, instead of trying to keep them all maintained in a single class. So maintain a Map<extId, Map<scope, ScopedEnvironmentVariableCollection>> in BaseExtHostTerminalService
There was a problem hiding this comment.
Perhaps I misunderstand, but that would require to change how we consume it all around the codebase, so I'm not fond of the idea. For eg.
Everywhere it is assumed collections is Map<extId, UnifiedExtensionCollectionObject> instead of Map<extId, Map<scope, ScopedEnvironmentVariableCollection>>.
There was a problem hiding this comment.
Let’s keep going with what you’ve done. This is just the way I would have done it from the start but it looks like it would take a bunch of re-work. Let's make sure to have a bunch of tests around merging the scopes collections if they don't already exist.
| } | ||
|
|
||
| replace(variable: string, value: string, scope?: vscode.EnvironmentVariableScope): void { | ||
| replace(variable: string, value: string, scope: vscode.EnvironmentVariableScope | undefined): void { |
There was a problem hiding this comment.
Guessing you just haven't got around to removing the scope??
There was a problem hiding this comment.
Let me clarify my design, where I have no plans of removing scope internally.
EnvironmentVariableCollection class is used only created once for each extension and then is passed around, that carries information for all the scopes. As scope is not present in constructor, it is present in each method in the class as a parameter; We should probably rename it to ExtensionOwnedEnvVarCollection.
The external vscode.EnvironmentVariableCollection is different from this, it does not have scope as a parameter.
There was a problem hiding this comment.
We should probably rename it to ExtensionOwnedEnvVarCollection
Little confusing as they are by definition owned by an extension
There was a problem hiding this comment.
We can go something like the following if it's already clear that they're for a specific extension:
MergedExtensionEnvVarCollectionGlobalEnvironmentVariableCollectionExtensionSpecificEnvironmentVariableCollection
Basically we want to indicate that it is a unified environment variable collection carrying information for all scopes, for a specific extension.
There was a problem hiding this comment.
ExtensionOwnedEnvVarCollection seemed appropriate as it is being used exactly that way in here:
IExtensionOwnedEnvironmentVariableMutator(s) came from ExtensionOwnedEnvVarCollection.
There was a problem hiding this comment.
IExtensionOwnedEnvironmentVariableMutator is just a mutator object but with the extension id attached. The environment variable collection is extension owned, but the mutator objects are owned by the collection and usually lack this property.
UnifiedEnvironmentVariableCollection is my favorite, but I'm not sure we should be renaming it as I believe it's only not-unified in the extension API. Adding "unified" throughout all usages wouldn't add much
There was a problem hiding this comment.
I like MergedExtensionEnvVarCollection because that's what it is, but I agree it probably wouldn't add much. I'll add comments clarifying where it is defined.
There was a problem hiding this comment.
That's confusing though, merged environment variable collection is already a thing and the merging there means there is only a single value per mutator type + variable.
There was a problem hiding this comment.
We could remove the "Merge" part, ExtensionEnvironmentVariableCollection. I'll add comments regardless clarifying.
For #182069