Skip to content

Use argument in addition to folderUri when conditionally resolving some variables.#95483

Merged
alexr00 merged 6 commits intomicrosoft:masterfrom
bzarco:workspace-variable-evaluation-fix
Apr 23, 2020
Merged

Use argument in addition to folderUri when conditionally resolving some variables.#95483
alexr00 merged 6 commits intomicrosoft:masterfrom
bzarco:workspace-variable-evaluation-fix

Conversation

@bzarco
Copy link
Contributor

@bzarco bzarco commented Apr 16, 2020 •
edited
Loading

Some variables were conditionally resolved by first checking folderUri. This is wrong, as getFolderUri() may return a valid one even when folderUri is undefined (when argument is used).

This PR changes the behavior so that getFolderUri() is always called, falling back to the original alternative if it fails to find one. By doing this, calls to resolve variables that do not provide a folderUri (aka workspace folder) will still do the right thing when an argument is provided (e.g. ${relativeFile:folder}).

This PR fixes #95482

@msftclas
Copy link

msftclas commented Apr 16, 2020 •
edited
Loading

CLA assistant check
All CLA requirements met.

@bzarco bzarco force-pushed the workspace-variable-evaluation-fix branch 2 times, most recently from 7638b15 to 86038cc Compare April 16, 2020 22:30
@bzarco
Copy link
Contributor Author

bzarco commented Apr 17, 2020

Just wanted to note, if the resolution functions are called a lot without a folder root (I assume not), we could gate the try/catch with folderUri || argument to avoid any extra overhead. That will make it more brittle, as future versions of getFolderUri() could return a folderUri under different conditions.

Also, as I noted in #95482, it seems multi-root workspaces do not pass a folderUri when resolving launch configurations, but they do for tasks. This PR does not fix the case where ${relativeFile} is used in a multi-root workspace, since getFolderUri() will still fail. If the tasks behavior is the desired one (use first folder as default), we could potentially fix it here as well.

alexr00
alexr00 requested changes Apr 17, 2020
View reviewed changes
Copy link
Member

@alexr00 alexr00 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like a good change, just a few modifications!

@alexr00
Copy link
Member

alexr00 commented Apr 17, 2020

And thank you for the tests!

@bzarco
Copy link
Contributor Author

bzarco commented Apr 17, 2020

Thanks for the quick review! I can rebase and squash once you think it is ready.

@bzarco bzarco requested a review from alexr00 April 17, 2020 14:35
@bzarco bzarco changed the title Do not use folderUri to gate variable resolving. Use argument in addition to folderUri when conditionally resolving some variables. Apr 17, 2020
bzarco and others added 4 commits April 22, 2020 20:03
@bzarco
Do not use folderUri to gate variable resolving.
dddd4ee 
Some vars were conditionally resolved by first checking `folderUri`.
This is wrong, as `getFolderUri()` may return a valid one even when
`folderUri` is undefined (when `argument` is used).
@alexr00 @bzarco
Make it easier to review tests
6af618a
@bzarco
Use argument (in addition to folderUri) instead of try/catch.
fab7127
@bzarco
Rename missing argument.
e891298
@bzarco bzarco force-pushed the workspace-variable-evaluation-fix branch from 5979c37 to 1957a18 Compare April 23, 2020 00:16
@bzarco bzarco requested a review from alexr00 April 23, 2020 00:18
@bzarco bzarco force-pushed the workspace-variable-evaluation-fix branch from 1957a18 to 96be765 Compare April 23, 2020 00:50
alexr00
alexr00 approved these changes Apr 23, 2020
View reviewed changes
Copy link
Member

@alexr00 alexr00 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, thank you!

@alexr00 alexr00 merged commit 9450b5e into microsoft:master Apr 23, 2020
@alexr00 alexr00 added this to the April 2020 milestone Apr 23, 2020
@github-actions github-actions bot locked and limited conversation to collaborators Jun 7, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@alexr00 alexr00 alexr00 approved these changes

Assignees

@alexr00 alexr00

Labels

None yet

Projects

None yet

Milestone

April 2020

Development

Successfully merging this pull request may close these issues.

${relativeFile} and ${relativeFileDirname} in multi-root workspace launch configuration resolve to absolute paths.

3 participants

@bzarco @msftclas @alexr00