Group interpreters in interpreter quick picker using separators #18171
Conversation
| switch (envType) { | ||
| case EnvironmentType.Venv: | ||
| case EnvironmentType.Conda: | ||
| case EnvironmentType.VirtualEnv: | ||
| case EnvironmentType.VirtualEnvWrapper: | ||
| case EnvironmentType.Pipenv: | ||
| case EnvironmentType.Poetry: | ||
| return EnvTypeHeuristic.Global; | ||
| // The default case covers global environments. | ||
| // For now this includes: pyenv, Windows Store, Global, System and Unknown environment types. | ||
| default: | ||
| return EnvTypeHeuristic.GlobalInterpreters; | ||
| } |
There was a problem hiding this comment.
We want to make sure environments of the same type appear together, so they can be grouped. But in this all global environment types were assigned the same priority, so order was not guaranteed.
src/client/interpreter/configuration/environmentTypeComparer.ts
Outdated
Show resolved
Hide resolved
| EnvironmentType.VirtualEnvWrapper, | ||
| EnvironmentType.Venv, | ||
| EnvironmentType.VirtualEnv, | ||
| EnvironmentType.Conda, |
There was a problem hiding this comment.
Instead we assign each type a unique priority, conda being the last in the priority queue.
| return 1; | ||
| } | ||
|
|
||
| if (!isCondaEnvironment(a) && isCondaEnvironment(b)) { | ||
| if (isBaseCondaEnvironment(b)) { | ||
| return -1; | ||
| } |
There was a problem hiding this comment.
This is handled by getPrioritizedEnvironmentType() instead.
| const recommendedInterpreter = this.envTypeComparer.getRecommended(interpreters, workspaceUri?.folderUri); | ||
| if (!recommendedInterpreter) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
Refactored it out as we also need this info while showing the recommended interpreter in the list.
src/client/interpreter/configuration/interpreterSelector/commands/setInterpreter.ts
Show resolved
Hide resolved
| export function setEnvDisplayString(env: PythonEnvInfo): void { | ||
| env.display = buildEnvDisplayString(env); | ||
| env.detailedDisplayName = buildEnvDisplayString(env, true); |
There was a problem hiding this comment.
We do not need type info in the display name when group name already contains a type. For eg.
Whereas if it's the "Workspace" group for instance we need the type info as group name doesn't contain type:
Hence 2 properties to carry display name.
| // When recommending an intepreter for a workspace, we either want to return a local one | ||
| // or fallback on a globally-installed interpreter, and we don't want want to suggest a global environment | ||
| // because we would have to add a way to match environments to a workspace. | ||
| const workspaceUri = this.interpreterHelper.getActiveWorkspaceUri(resource); |
There was a problem hiding this comment.
Might be a good item for debt week, we can remove the dependency on the interpreterHelper. What we really need here is the workspace uri, which is the real data. instead of depending on the object that provides that data.
There was a problem hiding this comment.
Are you suggesting we directly query vscode.workspace.getWorkspaceFolder API instead of using interpreter helper? (and get rid of the interpreter helper)
Interpreter helper also handles the case when resource is undefined, I guess that's why it's useful.
src/client/interpreter/configuration/interpreterSelector/commands/setInterpreter.ts
Outdated
Show resolved
Hide resolved
src/client/interpreter/configuration/interpreterSelector/commands/setInterpreter.ts
Outdated
Show resolved
Hide resolved
| this.setRecommendedItem(interpreterSuggestions, resource); | ||
| suggestions.push(...interpreterSuggestions); | ||
| return suggestions; | ||
| } | ||
|
|
||
| private getSuggestions(resource: Resource): QuickPickType[] { | ||
| const workspaceFolder = this.workspaceService.getWorkspaceFolder(resource); | ||
| const items = this.interpreterSelector.getSuggestions(resource, !!this.interpreterService.refreshPromise); |
There was a problem hiding this comment.
Having to run this.interpreterSelector.getSuggestions twice looks a bit strange. How expensive is it?
There was a problem hiding this comment.
It simply gets value from cache and sorts it, so not expensive at all.
…osoft/vscode-python#18171) * Group interpreters in interpreter quick picker using separators * Do not group while the list is loading * Cleanup * Ensure full display name does not have arch for non-global type envs * tes * Recommend the same interpreter we autoselect * Fix env resikver tst * Fix display name testS * Fix status bar display * Fix autoselection tests * Fix interpreter selector tests * Fix sorting/do not prioritize conda envs * Recommend Pipenv & Poetry envs * Do not autoselect poetry/pipenv envs * Fix interpreter selector tests * Fix set interpreter tests * News * Add test for grouping * Events to add an environment are handled appropriately if a grouped list is present * Code reviews * Add a new group for recommended interpreter * Localize group names * Fix unit tests * Change quickpick list placeholder
Closes #17944
This PR changes interpreter sorting and autoselection a bit.