-
Notifications
You must be signed in to change notification settings - Fork 42
Update venv labeling to show [uv] when uv is available #810
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
Changes from 7 commits
27fd645
0859b4d
388eb78
9746b27
a232ff5
44f84dd
9861198
bb30e4f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -29,7 +29,7 @@ import { | |
| SetEnvironmentScope, | ||
| } from '../../api'; | ||
| import { PYTHON_EXTENSION_ID } from '../../common/constants'; | ||
| import { VenvManagerStrings } from '../../common/localize'; | ||
| import { VenvManagerCommonStrings, VenvManagerStringsNoUv } from '../../common/localize'; | ||
| import { traceError, traceWarn } from '../../common/logging'; | ||
| import { createDeferred, Deferred } from '../../common/utils/deferred'; | ||
| import { showErrorMessage, withProgress } from '../../common/window.apis'; | ||
|
|
@@ -66,7 +66,7 @@ export class VenvManager implements EnvironmentManager { | |
| public readonly onDidChangeEnvironments = this._onDidChangeEnvironments.event; | ||
|
|
||
| readonly name: string; | ||
| readonly displayName: string; | ||
| displayName: string; // Made mutable to update with uv detection | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @copilot Remove comment
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Removed the comment as requested. The code is now cleaner without the explanatory comment. Commit: bb30e4f |
||
| readonly preferredPackageManagerId: string; | ||
| readonly description?: string | undefined; | ||
| readonly tooltip?: string | MarkdownString | undefined; | ||
|
|
@@ -83,7 +83,7 @@ export class VenvManager implements EnvironmentManager { | |
| // Descriptions were a bit too visually noisy | ||
| // https://github.com/microsoft/vscode-python-environments/issues/167 | ||
| this.description = undefined; | ||
| this.tooltip = new MarkdownString(VenvManagerStrings.venvManagerDescription, true); | ||
| this.tooltip = new MarkdownString(VenvManagerStringsNoUv.venvManagerDescription, true); | ||
| this.preferredPackageManagerId = 'ms-python.python:pip'; | ||
| this.iconPath = new ThemeIcon('python'); | ||
| } | ||
|
|
@@ -97,7 +97,7 @@ export class VenvManager implements EnvironmentManager { | |
| this._initialized = createDeferred(); | ||
|
|
||
| try { | ||
| await this.internalRefresh(undefined, false, VenvManagerStrings.venvInitialize); | ||
| await this.internalRefresh(undefined, false, VenvManagerCommonStrings.venvInitialize); | ||
| } finally { | ||
| this._initialized.resolve(); | ||
| } | ||
|
|
@@ -119,6 +119,14 @@ export class VenvManager implements EnvironmentManager { | |
| }; | ||
| } | ||
|
|
||
| /** | ||
| * Updates the display name for this environment manager. | ||
| * @param name The new display name to set. | ||
| */ | ||
| public setDisplayName(name: string): void { | ||
| this.displayName = name; | ||
| } | ||
|
|
||
| async create( | ||
| scope: CreateEnvironmentScope, | ||
| options: CreateEnvironmentOptions | undefined, | ||
|
|
@@ -148,15 +156,15 @@ export class VenvManager implements EnvironmentManager { | |
| // error on missing information | ||
| if (!this.globalEnv) { | ||
| this.log.error('No base python found'); | ||
| showErrorMessage(VenvManagerStrings.venvErrorNoBasePython); | ||
| showErrorMessage(VenvManagerCommonStrings.venvErrorNoBasePython); | ||
| throw new Error('No base python found'); | ||
| } | ||
| if (!this.globalEnv.version.startsWith('3.')) { | ||
| this.log.error('Did not find any base python 3.*'); | ||
| globals.forEach((e, i) => { | ||
| this.log.error(`${i}: ${e.version} : ${e.environmentPath.fsPath}`); | ||
| }); | ||
| showErrorMessage(VenvManagerStrings.venvErrorNoPython3); | ||
| showErrorMessage(VenvManagerCommonStrings.venvErrorNoPython3); | ||
| throw new Error('Did not find any base python 3.*'); | ||
| } | ||
| if (this.globalEnv && this.globalEnv.version.startsWith('3.')) { | ||
|
|
@@ -284,14 +292,14 @@ export class VenvManager implements EnvironmentManager { | |
| } | ||
|
|
||
| async refresh(scope: RefreshEnvironmentsScope): Promise<void> { | ||
| return this.internalRefresh(scope, true, VenvManagerStrings.venvRefreshing); | ||
| return this.internalRefresh(scope, true, VenvManagerCommonStrings.venvRefreshing); | ||
| } | ||
|
|
||
| async watcherRefresh(): Promise<void> { | ||
| if (this.skipWatcherRefresh) { | ||
| return; | ||
| } | ||
| return this.internalRefresh(undefined, true, VenvManagerStrings.venvRefreshing); | ||
| return this.internalRefresh(undefined, true, VenvManagerCommonStrings.venvRefreshing); | ||
| } | ||
|
|
||
| private async internalRefresh( | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -4,7 +4,7 @@ import * as path from 'path'; | |||||||||||||||||||||||||||||||||
| import { l10n, LogOutputChannel, ProgressLocation, QuickPickItem, QuickPickItemKind, ThemeIcon, Uri } from 'vscode'; | ||||||||||||||||||||||||||||||||||
| import { EnvironmentManager, PythonEnvironment, PythonEnvironmentApi, PythonEnvironmentInfo } from '../../api'; | ||||||||||||||||||||||||||||||||||
| import { ENVS_EXTENSION_ID } from '../../common/constants'; | ||||||||||||||||||||||||||||||||||
| import { Common, VenvManagerStrings } from '../../common/localize'; | ||||||||||||||||||||||||||||||||||
| import { Common, VenvManagerCommonStrings, VenvManagerStrings, VenvManagerStringsNoUv } from '../../common/localize'; | ||||||||||||||||||||||||||||||||||
| import { traceInfo } from '../../common/logging'; | ||||||||||||||||||||||||||||||||||
| import { getWorkspacePersistentState } from '../../common/persistentState'; | ||||||||||||||||||||||||||||||||||
| import { pickEnvironmentFrom } from '../../common/pickers/environments'; | ||||||||||||||||||||||||||||||||||
|
|
@@ -209,15 +209,15 @@ export async function getGlobalVenvLocation(): Promise<Uri | undefined> { | |||||||||||||||||||||||||||||||||
| const items: FolderQuickPickItem[] = [ | ||||||||||||||||||||||||||||||||||
| { | ||||||||||||||||||||||||||||||||||
| label: Common.browse, | ||||||||||||||||||||||||||||||||||
| description: VenvManagerStrings.venvGlobalFolder, | ||||||||||||||||||||||||||||||||||
| description: VenvManagerCommonStrings.venvGlobalFolder, | ||||||||||||||||||||||||||||||||||
| }, | ||||||||||||||||||||||||||||||||||
| ]; | ||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
| const venvPaths = getVenvFoldersSetting(); | ||||||||||||||||||||||||||||||||||
| if (venvPaths.length > 0) { | ||||||||||||||||||||||||||||||||||
| items.push( | ||||||||||||||||||||||||||||||||||
| { | ||||||||||||||||||||||||||||||||||
| label: VenvManagerStrings.venvGlobalFoldersSetting, | ||||||||||||||||||||||||||||||||||
| label: VenvManagerCommonStrings.venvGlobalFoldersSetting, | ||||||||||||||||||||||||||||||||||
| kind: QuickPickItemKind.Separator, | ||||||||||||||||||||||||||||||||||
| }, | ||||||||||||||||||||||||||||||||||
| ...venvPaths.map((p) => ({ | ||||||||||||||||||||||||||||||||||
|
|
@@ -243,7 +243,7 @@ export async function getGlobalVenvLocation(): Promise<Uri | undefined> { | |||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
| const selected = await showQuickPick(items, { | ||||||||||||||||||||||||||||||||||
| placeHolder: VenvManagerStrings.venvGlobalFolder, | ||||||||||||||||||||||||||||||||||
| placeHolder: VenvManagerCommonStrings.venvGlobalFolder, | ||||||||||||||||||||||||||||||||||
| ignoreFocusOut: true, | ||||||||||||||||||||||||||||||||||
| }); | ||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
|
|
@@ -269,24 +269,24 @@ async function createWithCustomization(version: string): Promise<boolean | undef | |||||||||||||||||||||||||||||||||
| const selection: QuickPickItem | undefined = await showQuickPick( | ||||||||||||||||||||||||||||||||||
| [ | ||||||||||||||||||||||||||||||||||
| { | ||||||||||||||||||||||||||||||||||
| label: VenvManagerStrings.quickCreate, | ||||||||||||||||||||||||||||||||||
| description: VenvManagerStrings.quickCreateDescription, | ||||||||||||||||||||||||||||||||||
| label: VenvManagerCommonStrings.quickCreate, | ||||||||||||||||||||||||||||||||||
| description: VenvManagerStringsNoUv.quickCreateDescription, | ||||||||||||||||||||||||||||||||||
| detail: l10n.t('Uses Python version {0} and installs workspace dependencies.', version), | ||||||||||||||||||||||||||||||||||
| }, | ||||||||||||||||||||||||||||||||||
| { | ||||||||||||||||||||||||||||||||||
| label: VenvManagerStrings.customize, | ||||||||||||||||||||||||||||||||||
| label: VenvManagerCommonStrings.customize, | ||||||||||||||||||||||||||||||||||
| description: VenvManagerStrings.customizeDescription, | ||||||||||||||||||||||||||||||||||
| }, | ||||||||||||||||||||||||||||||||||
| ], | ||||||||||||||||||||||||||||||||||
| { | ||||||||||||||||||||||||||||||||||
| placeHolder: VenvManagerStrings.selectQuickOrCustomize, | ||||||||||||||||||||||||||||||||||
| placeHolder: VenvManagerCommonStrings.selectQuickOrCustomize, | ||||||||||||||||||||||||||||||||||
| ignoreFocusOut: true, | ||||||||||||||||||||||||||||||||||
| }, | ||||||||||||||||||||||||||||||||||
| ); | ||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
| if (selection === undefined) { | ||||||||||||||||||||||||||||||||||
| return undefined; | ||||||||||||||||||||||||||||||||||
| } else if (selection.label === VenvManagerStrings.quickCreate) { | ||||||||||||||||||||||||||||||||||
| } else if (selection.label === VenvManagerCommonStrings.quickCreate) { | ||||||||||||||||||||||||||||||||||
| return false; | ||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||
| return true; | ||||||||||||||||||||||||||||||||||
|
|
@@ -305,19 +305,27 @@ async function createWithProgress( | |||||||||||||||||||||||||||||||||
| const pythonPath = | ||||||||||||||||||||||||||||||||||
| os.platform() === 'win32' ? path.join(envPath, 'Scripts', 'python.exe') : path.join(envPath, 'bin', 'python'); | ||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
| const useUv = await isUvInstalled(log); | ||||||||||||||||||||||||||||||||||
| const progressTitle = useUv | ||||||||||||||||||||||||||||||||||
| ? l10n.t( | ||||||||||||||||||||||||||||||||||
| 'Creating virtual environment named {0} using python version {1} [uv].', | ||||||||||||||||||||||||||||||||||
| path.basename(envPath), | ||||||||||||||||||||||||||||||||||
| basePython.version, | ||||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||||
| : l10n.t( | ||||||||||||||||||||||||||||||||||
| 'Creating virtual environment named {0} using python version {1}.', | ||||||||||||||||||||||||||||||||||
| path.basename(envPath), | ||||||||||||||||||||||||||||||||||
| basePython.version, | ||||||||||||||||||||||||||||||||||
| ); | ||||||||||||||||||||||||||||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @copilot may be something like this, more compact?
Suggested change
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Implemented your suggestion for a more compact approach using a single l10n.t() call with conditional text. This eliminates the duplicate string and makes the code cleaner. Commit: bb30e4f |
||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
| return await withProgress( | ||||||||||||||||||||||||||||||||||
| { | ||||||||||||||||||||||||||||||||||
| location: ProgressLocation.Notification, | ||||||||||||||||||||||||||||||||||
| title: l10n.t( | ||||||||||||||||||||||||||||||||||
| 'Creating virtual environment named {0} using python version {1}.', | ||||||||||||||||||||||||||||||||||
| path.basename(envPath), | ||||||||||||||||||||||||||||||||||
| basePython.version, | ||||||||||||||||||||||||||||||||||
| ), | ||||||||||||||||||||||||||||||||||
| title: progressTitle, | ||||||||||||||||||||||||||||||||||
| }, | ||||||||||||||||||||||||||||||||||
| async () => { | ||||||||||||||||||||||||||||||||||
| const result: CreateEnvironmentResult = {}; | ||||||||||||||||||||||||||||||||||
| try { | ||||||||||||||||||||||||||||||||||
| const useUv = await isUvInstalled(log); | ||||||||||||||||||||||||||||||||||
| // env creation | ||||||||||||||||||||||||||||||||||
| if (basePython.execInfo?.run.executable) { | ||||||||||||||||||||||||||||||||||
| if (useUv) { | ||||||||||||||||||||||||||||||||||
|
|
@@ -353,13 +361,19 @@ async function createWithProgress( | |||||||||||||||||||||||||||||||||
| }); | ||||||||||||||||||||||||||||||||||
| } catch (e) { | ||||||||||||||||||||||||||||||||||
| // error occurred while installing packages | ||||||||||||||||||||||||||||||||||
| result.pkgInstallationErr = e instanceof Error ? e.message : String(e); | ||||||||||||||||||||||||||||||||||
| result.pkgInstallationErr = useUv | ||||||||||||||||||||||||||||||||||
| ? `Failed to install packages with uv: ${e instanceof Error ? e.message : String(e)}` | ||||||||||||||||||||||||||||||||||
| : e instanceof Error | ||||||||||||||||||||||||||||||||||
| ? e.message | ||||||||||||||||||||||||||||||||||
| : String(e); | ||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||
| result.environment = env; | ||||||||||||||||||||||||||||||||||
| } catch (e) { | ||||||||||||||||||||||||||||||||||
| log.error(`Failed to create virtual environment: ${e}`); | ||||||||||||||||||||||||||||||||||
| result.envCreationErr = `Failed to create virtual environment: ${e}`; | ||||||||||||||||||||||||||||||||||
| result.envCreationErr = useUv | ||||||||||||||||||||||||||||||||||
| ? `Failed to create virtual environment with uv: ${e}` | ||||||||||||||||||||||||||||||||||
| : `Failed to create virtual environment: ${e}`; | ||||||||||||||||||||||||||||||||||
| log.error(result.envCreationErr); | ||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||
| return result; | ||||||||||||||||||||||||||||||||||
| }, | ||||||||||||||||||||||||||||||||||
|
|
@@ -369,14 +383,14 @@ async function createWithProgress( | |||||||||||||||||||||||||||||||||
| export function ensureGlobalEnv(basePythons: PythonEnvironment[], log: LogOutputChannel): PythonEnvironment[] { | ||||||||||||||||||||||||||||||||||
| if (basePythons.length === 0) { | ||||||||||||||||||||||||||||||||||
| log.error('No base python found'); | ||||||||||||||||||||||||||||||||||
| showErrorMessage(VenvManagerStrings.venvErrorNoBasePython); | ||||||||||||||||||||||||||||||||||
| showErrorMessage(VenvManagerCommonStrings.venvErrorNoBasePython); | ||||||||||||||||||||||||||||||||||
| throw new Error('No base python found'); | ||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
| const filtered = basePythons.filter((e) => e.version.startsWith('3.')); | ||||||||||||||||||||||||||||||||||
| if (filtered.length === 0) { | ||||||||||||||||||||||||||||||||||
| log.error('Did not find any base python 3.*'); | ||||||||||||||||||||||||||||||||||
| showErrorMessage(VenvManagerStrings.venvErrorNoPython3); | ||||||||||||||||||||||||||||||||||
| showErrorMessage(VenvManagerCommonStrings.venvErrorNoPython3); | ||||||||||||||||||||||||||||||||||
| basePythons.forEach((e, i) => { | ||||||||||||||||||||||||||||||||||
| log.error(`${i}: ${e.version} : ${e.environmentPath.fsPath}`); | ||||||||||||||||||||||||||||||||||
| }); | ||||||||||||||||||||||||||||||||||
|
|
@@ -457,15 +471,15 @@ export async function createPythonVenv( | |||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
| const name = await showInputBox({ | ||||||||||||||||||||||||||||||||||
| prompt: VenvManagerStrings.venvName, | ||||||||||||||||||||||||||||||||||
| prompt: VenvManagerCommonStrings.venvName, | ||||||||||||||||||||||||||||||||||
| value: '.venv', | ||||||||||||||||||||||||||||||||||
| ignoreFocusOut: true, | ||||||||||||||||||||||||||||||||||
| validateInput: async (value) => { | ||||||||||||||||||||||||||||||||||
| if (!value) { | ||||||||||||||||||||||||||||||||||
| return VenvManagerStrings.venvNameErrorEmpty; | ||||||||||||||||||||||||||||||||||
| return VenvManagerCommonStrings.venvNameErrorEmpty; | ||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||
| if (await fsapi.pathExists(path.join(venvRoot.fsPath, value))) { | ||||||||||||||||||||||||||||||||||
| return VenvManagerStrings.venvNameErrorExists; | ||||||||||||||||||||||||||||||||||
| return VenvManagerCommonStrings.venvNameErrorExists; | ||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||
| }, | ||||||||||||||||||||||||||||||||||
| }); | ||||||||||||||||||||||||||||||||||
|
|
@@ -513,15 +527,15 @@ export async function removeVenv(environment: PythonEnvironment, log: LogOutputC | |||||||||||||||||||||||||||||||||
| const result = await withProgress( | ||||||||||||||||||||||||||||||||||
| { | ||||||||||||||||||||||||||||||||||
| location: ProgressLocation.Notification, | ||||||||||||||||||||||||||||||||||
| title: VenvManagerStrings.venvRemoving, | ||||||||||||||||||||||||||||||||||
| title: VenvManagerCommonStrings.venvRemoving, | ||||||||||||||||||||||||||||||||||
| }, | ||||||||||||||||||||||||||||||||||
| async () => { | ||||||||||||||||||||||||||||||||||
| try { | ||||||||||||||||||||||||||||||||||
| await fsapi.remove(envPath); | ||||||||||||||||||||||||||||||||||
| return true; | ||||||||||||||||||||||||||||||||||
| } catch (e) { | ||||||||||||||||||||||||||||||||||
| log.error(`Failed to remove virtual environment: ${e}`); | ||||||||||||||||||||||||||||||||||
| showErrorMessage(VenvManagerStrings.venvRemoveFailed); | ||||||||||||||||||||||||||||||||||
| showErrorMessage(VenvManagerCommonStrings.venvRemoveFailed); | ||||||||||||||||||||||||||||||||||
| return false; | ||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||
| }, | ||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
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.
@karthiknadig can you just review this to make sure this structure of setting strings makes sense and follows good coding practices
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 actually looks a bit weird. There are several string duplicates.
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.
@karthiknadig moved to duplicates to a diff function so there shouldn't be repeats. Lmk your thoughts