Switch to using URIs as much as possible #9641
Conversation
Codecov Report
@@ Coverage Diff @@
## main #9641 +/- ##
======================================
- Coverage 73% 63% -10%
======================================
Files 194 201 +7
Lines 8428 9825 +1397
Branches 1231 1557 +326
======================================
+ Hits 6175 6248 +73
- Misses 1777 3079 +1302
- Partials 476 498 +22
|
src/kernels/jupyter/interpreter/jupyterInterpreterOldCacheStateStore.node.ts
Outdated
Show resolved
Hide resolved
| return path.join(this.pathUtils.home, macJupyterPath); | ||
| } else { | ||
| return path.join(this.pathUtils.home, linuxJupyterPath); | ||
| const userHomeDir = getUserHomeDir(); |
There was a problem hiding this comment.
Should be 1 location to get the user home dir now. We had like 3 before.
| if (getOSType() === OSType.Windows) { | ||
| return getEnvironmentVariable('USERPROFILE'); | ||
| return fsPathToUri(getEnvironmentVariable('USERPROFILE') || homePath); |
There was a problem hiding this comment.
Without the homePath stuff tests will fail on windows (for linux based tests)
src/kernels/types.ts
Outdated
| @@ -319,7 +320,7 @@ export interface IJupyterKernelSpec { | |||
| id?: string; | |||
| name: string; | |||
| language?: string; | |||
| path: string; | |||
| path: Uri; | |||
There was a problem hiding this comment.
Would a rename be considered here? This now shows up in some places like kernel.path.path.
There was a problem hiding this comment.
I think that's more what I would expect. But a variable called path also being a Uri wouldn't really surprise me, so feel free to change or not based on what you think. Not a big diff for me.
src/platform/api/extension.d.ts
Outdated
| @@ -57,12 +57,12 @@ export type PythonVersion = { | |||
| }; | |||
| export type PythonEnvironment = { | |||
| displayName?: string; | |||
| path: string; | |||
| path: Uri; | |||
There was a problem hiding this comment.
Another possible path=>Uri name for the member.
| @@ -4,6 +4,7 @@ | |||
| 'use strict'; | |||
|
|
|||
| import * as path from '../platform/vscode-path/path'; | |||
There was a problem hiding this comment.
Do we have linters to ensure nodejs module path never gets used.
Or would we want to just replace path with vscode-path in webpack?
I'd think one of the above would be necessary to ensure we don't use path in web files.
There was a problem hiding this comment.
We do have a linter that ensures no more usage of 'path' from nodejs anywhere except for '.node' files.
| @@ -1205,7 +1218,7 @@ function compareAgainstKernelDisplayNameInNotebookMetadata( | |||
| argv: [], | |||
| display_name: notebookMetadata.kernelspec.display_name, | |||
| name: notebookMetadata.kernelspec.name, | |||
| path: '' | |||
| uri: Uri.file('') | |||
There was a problem hiding this comment.
This feels very risky. I think we should mark path as a nullable property
we could have code paths where we check if path is empty e.g. if (path),
Such a condition would evaluate to false as the string is empty, now these would never show up in compilations and they'd always be true because its always an object and never an empty string.
There was a problem hiding this comment.
Those were all handled with the rename. They wouldn't compile.
There was a problem hiding this comment.
Cause before it used to have stuff like this:
interpreter || { path: string }.
Now when path is accessed it says interpreter doesn't have it.
The only risk here is if we had forced things to 'any'.
There was a problem hiding this comment.
And well our integration tests and unit tests would likely have caught it.
There was a problem hiding this comment.
InterpreterOrUri was another example of where it was checking for '.path'. After the change of .path to a URI those still worked. After this rename, these turned into a compile time error and were actually correct. So this rename was necessary to catch that usage.
Fixes #9599