-
Notifications
You must be signed in to change notification settings - Fork 162
[Language Service] Fix project path on notebooks when running on Windows #2872
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
| # we have VS Code Web + Jupyter support. | ||
| normalized_root = os.path.normpath(os.path.join(os.getcwd(), project_root)) | ||
| self._config["projectRoot"] = "file://" + normalized_root | ||
| self._config["projectRoot"] = "file:///" + normalized_root |
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.
My VS-Code instance was confusing the drive c: with the authority of the URI. The URI spec says that the way to specify an empty authority is by doing "file:///path"
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 seems odd in that on Mac and Linux the path will already start with a forward-slash. Having the extra slash may be benign, but also isn't correct.
| // Rust expects paths, not encoded URIs, so we need to decode here. | ||
| // We need to decode the components and then the URI. | ||
| decodeURI( | ||
| decodeURIComponent(Utils.joinPath(uriToQuery, name).toString()), |
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.
Are there risks here when not working with file system paths (e.g. when we load files from github, or in the web experience)?
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.
Is this just solving the same problem you already had to do recently for the # char @minestarks ? This look safe to you?
| const mappedFiles: [string, vscode.FileType][] = fileSearchResult.map( | ||
| ([name, type]) => [Utils.joinPath(uriToQuery, name).toString(), type], | ||
| ([name, type]) => [ | ||
| // Rust expects paths, not encoded URIs, so we need to decode here. |
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.
Rust expects paths
I don't think this is right - in general, all the paths in the language service should be URIs coming directly from VS Code. Round tripping VS Code-generated Uris with with vscode.Uri.toString() and vscode.Uri.parse() should work fine without any need for decoding
Fixes #2866