Skip to content

Use resolveAsync in terminal Variable Resolver #120328

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

Closed
alexr00 opened this issue Apr 1, 2021 · 3 comments · Fixed by #121918
Closed

Use resolveAsync in terminal Variable Resolver #120328

alexr00 opened this issue Apr 1, 2021 · 3 comments · Fixed by #121918
Assignees
Labels
debt Code quality issues important Issue identified as high-priority insiders-released Patch has been released in VS Code Insiders perf-startup terminal General terminal issues that don't fall under another label
Milestone

Comments

@alexr00
Copy link
Member

alexr00 commented Apr 1, 2021

With #108804 we are trying to remove process.env from the configuration resolver service. The new way to get the environment is async and requires that the all the configuration resolver methods be async. I have started the adoption of the new async methods in #120326, but there's one use in terminal that looks like it will require more work:

export function createVariableResolver(lastActiveWorkspace: IWorkspaceFolder | undefined, configurationResolverService: IConfigurationResolverService | undefined): VariableResolver | undefined {
if (!configurationResolverService) {
return undefined;
}
return (str) => configurationResolverService.resolve(lastActiveWorkspace, str);
}

Option 1: Convert that use of resolve to use resolveAsync.
Option 2: I provide a new resolveWithEnv method on the configuration resolver service that is sync, but requires you to pass in your own IProcessEnvironment.

Which option makes the most sense here?

@alexr00 alexr00 added important Issue identified as high-priority debt Code quality issues labels Apr 1, 2021
@Tyriar
Copy link
Member

Tyriar commented Apr 1, 2021

I think the usage of VariableResolver in extHostTerminalService must remain sync since it's used as part of the extension API. Is this a problem there since process.env is fine to use? The main purpose of this VariableResolve type was to share code between variable processes.

@alexr00
Copy link
Member Author

alexr00 commented Apr 6, 2021

Sounds like Option 2 is the way to go then. I will add a new resolveWithEnv method that is sync and lets you pass in your own IProcessEnvironment.

alexr00 added a commit that referenced this issue Apr 6, 2021
@alexr00
Copy link
Member Author

alexr00 commented Apr 6, 2021

Added:

resolveWithEnvironment(environment: IProcessEnvironment, folder: IWorkspaceFolder | undefined, value: string): string;

will that let you keep your code reuse?

digitarald pushed a commit that referenced this issue Apr 8, 2021
gjsjohnmurray pushed a commit to gjsjohnmurray/vscode that referenced this issue Apr 8, 2021
@Tyriar Tyriar added this to the April 2021 milestone Apr 22, 2021
@Tyriar Tyriar added the terminal General terminal issues that don't fall under another label label Apr 22, 2021
Tyriar added a commit that referenced this issue Apr 22, 2021
@github-actions github-actions bot locked and limited conversation to collaborators Jun 6, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
debt Code quality issues important Issue identified as high-priority insiders-released Patch has been released in VS Code Insiders perf-startup terminal General terminal issues that don't fall under another label
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants
@Tyriar @meganrogge @alexr00 and others