-
Notifications
You must be signed in to change notification settings - Fork 30.6k
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
Resolve and replace commands for next command #92697
Resolve and replace commands for next command #92697
Conversation
@davidruhmann what is exactly the approach behind your change? (Don't assume that JSON properties are ordered. JSON objects are dictionaries, not sequential "programs") |
Definitely. Appreciate your taking a look at this. My approach to the change was to be as backwards compatible as possible and not change the input or output of the public functions. All the changes culminate around making this line pass in an object with the known replacements inserted.
So when this
Previously, the object passed into the command execution would be the raw original without replacement values.
As for documentation updates, I do not think any updates are needed; as the current documentation makes it sound like how it works with my changes is how it should work.
Please let me know if I have missed something or need to update anything else. |
const variableValues: IStringDictionary<string> = Object.create(null); | ||
|
||
for (const variable of variables) { | ||
const config = deepClone(configuration); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Make a clone of the configuration so we do not manipulate the object passed into this function.
@@ -184,7 +182,7 @@ export abstract class BaseConfigurationResolverService extends AbstractVariableR | |||
case 'command': | |||
// use the name as a command ID #12735 | |||
const commandId = (variableToCommandMap ? variableToCommandMap[name] : undefined) || name; | |||
result = await this.commandService.executeCommand(commandId, configuration); | |||
result = await this.commandService.executeCommand(commandId, object); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The object passed into executeCommand now has all the previously evaluated replacements inserted.
variables.push(command); | ||
const found = command in variables; | ||
if (!found) { | ||
// NOTE this if assumes that the user will only ever want one computed value. (e.g. have two of the same command but only get the first output for both) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This assumption was from the existing code. Just added a comment to make it clear while I was making changes.
@weinand Also have some ideas on how to resolve the ordering that I will play with. Do you have any specific thoughts around it? Some of my initial thoughts:
|
@davidruhmann I've reopened issue #89758 Let's continue the discussion there and please verify that my take of the problem is correct. |
@davidruhmann Thanks a lot for the PR, but we do not plan to solve #89758 on the VS Code side. Please see my comment |
This PR fixes #89758
Which will add support for the use case of using command and environment values in the configuration as they are resolved for subsequent command executions. This enables the following use case.
resolveDebugConfigurationWithSubstitutedVariables
does not work for this use case as both commands are resolved/executed before replacement occurs.${command:pickRemoteProcess}
needs the value from${command:vscode-docker.containers.select}
in the config object passed to it during execution.