Run configurationResolver on terminal env vars #40059
Conversation
| // Merge process env with the env from config | ||
| const envFromConfig = { ...process.env }; | ||
| const lastActiveWorkspaceRootUri = this._historyService.getLastActiveWorkspaceRoot('file'); | ||
| const lastActiveWorkspaceRoot = this._workspaceContextService.getWorkspaceFolder(lastActiveWorkspaceRootUri); |
There was a problem hiding this comment.
Let's move this variable down to the other chunk to make it clear that it's not used in the following line.
| const envSettingKey = platform.isWindows ? 'windows' : (platform.isMacintosh ? 'osx' : 'linux'); | ||
| TerminalInstance.mergeEnvironments(envFromConfig, this._configHelper.config.env[envSettingKey]); | ||
| const envFromConfig = { ...this._configHelper.config.env[envSettingKey] }; | ||
| Object.keys(envFromConfig).forEach((key) => { |
There was a problem hiding this comment.
This section's getting a bit crazy, how about we organize it like this:
mergeEnvironmentsnow takes a base environment and an array of secondary environments- The secondary environments are filtered through a
resolveConfigurationVariables
I think we could then minimize the number of lines dealing with the env in this function and de-duplicate the _configurationResolverService parts. What do you think?
There was a problem hiding this comment.
I thought exactly that about the mergeEnvironments. But, one issue with that though is that mergeEnvironments is static. Resolving of variables need the configurationResolverService and the workspaceRoot
There was a problem hiding this comment.
If changing the static is too much work then you can pass in _configurationResolveService as a param.
| // TODO: locale should not be optional | ||
| public static createTerminalEnv(parentEnv: IStringDictionary<string>, shell: IShellLaunchConfig, cwd: string, locale?: string, cols?: number, rows?: number): IStringDictionary<string> { | ||
| protected createTerminalEnv(parentEnv: IStringDictionary<string>, shell: IShellLaunchConfig, cwd: string, locale?: string, cols?: number, rows?: number): IStringDictionary<string> { |
There was a problem hiding this comment.
@Tyriar How about making createTerminalEnv non static?
There was a problem hiding this comment.
I'm all for that, the only reason it's still like that is because the tests were written that way. It would be better to do protected and inherit in a super testing class. You would probably have to block out the terminal process launching stuff from launching during tests for that to work though which might be too much work for this PR.
There was a problem hiding this comment.
In that case, I'll revert that change
ramya-rao-a
force-pushed
the
resolve-terminal-env-vars
branch
from
9b441c4 to
2e4a98b
Compare
|
Moved the variable resolving into its own function and tested, looks good 👌
|
|
Good to merge after CI passes |
Fixes #34337