-
Notifications
You must be signed in to change notification settings - Fork 34
Fix workspaceFolder name resolution in multi-root configs #240
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
base: main
Are you sure you want to change the base?
Conversation
|
@bwateratmsft I'll update the PR with the feedback you provided! |
bwateratmsft
left a comment
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.
(Just changing the review status to clean my dashboard for now 😄)
|
@bwateratmsft I'll update this PR in the next day or two, sorry for the delay! |
|
No worries! |
Formatting Commit per reviewer feedback Co-authored-by: Brandon Waterloo [MSFT] <[email protected]>
|
@bwateratmsft All feedback should be implemented and pushed now! |
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.
Looks great! I will try to give it a spin this week and merge it.
Note to self: need to check if this also fixes #103, though I suspect not due to the hardcoded ${workspaceFolder} here: https://github.com/microsoft/vscode-docker/blob/6893922e3afbe73a95b4907e3570b0ef27eb154f/src/debugging/netcore/NetCoreDebugHelper.ts#L175 (and in a lot of other places)
|
@bwateratmsft sounds good! If I need to make any other changes, just let me know! |
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.
So unfortunately I don't think this fully fixes #101. The launch gets farther along, but still ends up hitting the same error as #103.
In #103, there is a problem line here, and a few places below it as well:
| pipeCwd: '${workspaceFolder}', |
In short, we need to resolve ${workspaceFolder:foo} instead of ${workspaceFolder} there. However, I don't want to do that when there is only the one workspace folder--that's probably going to result in problems.
You could make a utility method that takes the selected vscode.WorkspaceFolder as input, and if it's the only one just returns ${workspaceFolder}, but if there are multiple then it returns ${workspaceFolder:foo}. We would need to verify that the behavior is still correct when you have a workspace open, but only one workspace folder within it. You can probably use workspaceFile and workspaceFolders here to reason out what should be returned: https://code.visualstudio.com/api/references/vscode-api#workspace
The catch is, I wouldn't want to make this change for .NET only; we really should fix all our uses of ${workspaceFolder}. It's acceptable as a default value in things that get scaffolded, but shouldn't be in a non-user-overridable place like the problem lines linked above.
The other catch is, if the tasks are defined within the workspace folder, that might also cause problems to change to ${workspaceFolder:foo}. In general, tasks have a scope (universal, workspace, or workspace folder). Universal is probably a non-issue here, but we need to make sure we resolve the right thing at the right scope.
Co-authored-by: Brandon Waterloo [MSFT] <[email protected]>
|
@bwateratmsft thanks for all your feedback, I'll take a closer look into this soon. |
Summary
Rationale
Changes
resolveVariablesto look up workspace folders by name and normalize the returned pathresolveVariablesunit tests exercising scoped name resolution, fallback, and unknown folder retentionFixes #101