-
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
Sort available environments by assumed usefulness #16559
Sort available environments by assumed usefulness #16559
Conversation
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.
Other than the path comparison thing this looks good.
|
||
switch (envType) { | ||
case EnvironmentType.Venv: { | ||
if (workspacePath.length > 0 && environment.envPath?.startsWith(workspacePath)) { |
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.
Paths probably should be normalized before comparing.
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 helper should be useful,
vscode-python/src/client/common/platform/fs-paths.ts
Lines 151 to 158 in 488aaa2
/** | |
* Returns true if given file path exists within the given parent directory, false otherwise. | |
* @param filePath File path to check for | |
* @param parentPath The potential parent path to check for | |
*/ | |
export function isParentPath(filePath: string, parentPath: string): boolean { | |
return normCasePath(filePath).startsWith(normCasePath(parentPath)); | |
} |
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.
EnvTypeHeuristic.Local
can also be of other types, including Unknown
. I think we should be doing the check of whether an env is local prior to checking env types.
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.
EnvTypeHeuristic.Local can also be of other types, including Unknown. I think we should be doing the check of whether an env is local prior to checking env types.
So from what I understand you are suggesting that any environment path that has the workspace as the parent path should be considered local? How can an environment end up categorized as Unknown
again?
I think that if we failed to identify an environment it shouldn't be prioritized it over others.
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.
you are suggesting that any environment path that has the workspace as the parent path should be considered local?
Yes
How can an environment end up categorized as Unknown again?
If none of our environments type checks matches, a local environment can be classified as Unknown
.
vscode-python/src/client/pythonEnvironments/base/locators/lowLevel/workspaceVirtualEnvLocator.ts
Line 59 in a393225
return PythonEnvKind.Unknown; |
But practically speaking it shouldn't happen. So maybe we shouldn't worry about the Unknown
case in local envs.
I think that if we failed to identify an environment it shouldn't be prioritized it over others.
Or we can make an exception case as you suggested 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.
Besides Venv
and Unknown
, which other environment types could possibly be categorized as local?
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.
Poetry
, conda
can both have local and global versions.
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.
Virtualenv
, Pipenv
can be in there as well. Basically I don't think we should define local envs based on kind.
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.
LGTM otherwise
|
||
switch (envType) { | ||
case EnvironmentType.Venv: { | ||
if (workspacePath.length > 0 && environment.envPath?.startsWith(workspacePath)) { |
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.
EnvTypeHeuristic.Local
can also be of other types, including Unknown
. I think we should be doing the check of whether an env is local prior to checking env types.
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.
LGTM
For #16520