ability to evaluate variables in user/workspace settings

[DonJayamanne]

The requirement is simple; provide the ability to use variables such as ${workspaceRoot} and ${env.PATH} in settings.json.

The implemented in the pull request is definitely not the right approach.
I'm looking for a more architecturally sound solution, i.e. a discussion on how to best approach this.
Also, I'm making use of the SystemVariables class (variables such as ${file} and the like will not work, infact VS Code will throw an error if such values are used)

[mention-bot]

By analyzing the blame information on this pull request, we identified @egamma and @alexandrudima to be potential reviewers

[msftclas]

Hi @DonJayamanne, I'm your friendly neighborhood Microsoft Pull Request Bot (You can call me MSBOT). Thanks for your contribution!
You've already signed the contribution license agreement. Thanks!

The agreement was validated by Microsoft and real humans are currently evaluating your PR.

TTYL, MSBOT;

[bpasero]

The implemented in the pull request is definitely not the right approach.

That is not a sentence I like to read when I need to invest time to review the PR...

[DonJayamanne]

I agree with your comments. However, I was left with just one other option, which was to spend more time (few more days) on this issue and then try it.
Having had a chat with Kai, I was advised to take this approach, with the hope this would start a conversation that would help me figure out a better solution to the issue.

[kieferrm]

@bpasero A bit of context: This functionality would greatly help us to handle the management of Python virtual environments across teams that use different operating systems and different ways of managing their Python installations. Rather than filing a feature-request and wait for things to happen we'd like to provide that functionality as a pull request. However, the current approach doesn't seem right and thus we hoped you could nudge us in the right direction. What better way is there than to discuss concrete code?

[bpasero]

Makes sense. Maybe advocate for it to be put on the July plan then.

[bpasero]

I think it does not make sense to hook SystemVariables into the extHostConfiguration because that one is only being used from extensions and it would return different results from any client within other areas (e.g. workbench itself).

I would suggest to add SystemVariables into the configuration services we have (https://github.com/Microsoft/vscode/blob/master/src/vs/workbench/services/configuration/node/configurationService.ts#L28 and https://github.com/Microsoft/vscode/blob/ben/july/src/vs/platform/configuration/node/nodeConfigurationService.ts#L35) so that loading a configuration section always replaces variables properly.

Some issues:

• the dependency to editor service in SystemVariables cannot be met and should be made optional (configuration service gets created much earlier and cannot have a dependency to editor service)
• you have to check all places where we expose the configuration from the service, there are more than one (looks like https://github.com/Microsoft/vscode/blob/master/src/vs/platform/configuration/common/configuration.ts#L20, https://github.com/Microsoft/vscode/blob/master/src/vs/platform/configuration/common/configuration.ts#L26 and also from the event in https://github.com/Microsoft/vscode/blob/master/src/vs/platform/configuration/common/configuration.ts#L36)

It would also be good to add tests for this, maybe SystemVariables could be taken out of the node environment so that it can be created from a test context? The only dependency it has seems to be to process (for the environment) which I think can easily be just passed in.

[DonJayamanne]

Thanks for the feedback

[DonJayamanne]

@bpasero, thanks for your feedback, I have a few suggestions and questions.

I would suggest to add SystemVariables into the configuration services we have node/configurationService.ts#L28

Wouldn't the base class 'configuration/common/configurationService.ts' be the right place for the change? I believe it is.

I would suggest to add SystemVariables into the configuration services we have configuration/node/nodeConfigurationService.ts#L35

Agreed.
However, this class doesn't have access to any of the following:

• editor service
• workspace context service
• current workspace path

Unfortunately the SystemVariables class needs access to one of the above. I could fix this.

However, before I do this, I would like to understand the purpose/usage of nodeConfigurationService.ts.
So, please could you help me understand the purpose and/or the context within which this is used. Thanks

[bpasero]

Yes, that base class makes sense. And yes SystemVariables needs those optional dependencies. I think the workspace context service and thus the current workspace path is known at the time the configuration service gets created. That is pretty much the first service we create.

I think @joaomoreno introduced nodeConfigurationService, he should comment on its purpose. As far as I remember, he uses it to be able to install extensions from the command line.

[joaomoreno]

The NodeConfigurationService is a stepping stone towards having a workspace independent configuration service, needed by both the main and the shared processes. It does not nor never will have access to the editor service or anything workspace related. It only concerns the global configuration settings. @bpasero you know this, we talked about it.

[bpasero]

Given that it would probably make sense to have a workspace/editor independent SystemVariables instance that just allows to fill in process.env variables and have a subclass that is being used within the workbench.

@DonJayamanne given a lot of debt in the configuration service area I would probably hold on to making changes until we cleaned this mess up.

Related issues:

• Move core services from main back into shell #6093
• Should stop sending global settings and keybindings via IPC #6094
• IWorkspaceContextService should only exist on renderer side and have less functionality #6095

[DonJayamanne]

@bpasero , sure thing

[bpasero]

Configuration service is undergoing major refactorings. Closing this obsolete PR.