-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Use workspaceFolder token instead of workspaceRoot #267
Use workspaceFolder token instead of workspaceRoot #267
Conversation
Archive of 0.7.0
* 'master' of https://github.com/Microsoft/vscode-python: Fixes #56 list all environments (#219) Fixes #57 Disable activation on debugging (#220) Fixes #26 Do not run linters when linters are disabled (#222)
* upstream/master: Fix typo in README.md (#252) Disable linter without workspaces (#241)
@@ -39,6 +40,22 @@ suite('Multiroot Python Path Settings Updater', () => { | |||
assert.equal(folderValue, pythonPath, 'Workspace Python Path not updated'); | |||
}); | |||
|
|||
test('Updating Workspace Folder Python Path should work with relative paths', async () => { |
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.
Should there be an assert that the path is actually relative since it's opaque here in the code that it's relative? That way if workspace3Uri
changes we will know this test is no longer valid.
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.
Sorry, don't quite understand what you're saying.
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.
By relative, i mean the python path is relative. As you can see I'm prefixing the name of the python executable with a /
(meaning it is relative to current workspace).
const pythonExec = `/xWorkspacePythonPath${new Date().getMilliseconds()}`;
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 leaving off the slash makes it an absolute path? I guess what's tripping up my brain is that a leading /
on UNIX makes something absolute.
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.
Sorry, my mistake, will make the tests clearer. Don't want to explain it, hopefully you'll understand the comments and code, if not then i'll revise it.
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.
@brettcannon Fixed, please check again
@@ -58,14 +75,33 @@ suite('Multiroot Python Path Settings Updater', () => { | |||
assert.equal(folderValue, pythonPath, 'Workspace Python Path not updated'); | |||
}); | |||
|
|||
test('Python Path should be relative to workspace', async () => { | |||
// tslint:disable-next-line:no-invalid-template-strings | |||
test('Python Paths containing ${workspaceRoot should be resolved as ${workspaceFolder}', async () => { |
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.
${workspaceRoot}
(missing the close curly brace).
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.
Fixed
Fixes #258