Skip to content

Commit 083a1da

Browse files
karthiknadigkarthiknadig
committed
Address PR review comments: modal prompt, memoize computeId, update globalEnv, fix comments, add tests
1 parent e79c752 commit 083a1da

File tree

6 files changed

+109
-9
lines changed

6 files changed

+109
-9
lines changed

‎src/common/localize.ts‎

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -213,9 +213,11 @@ export namespace ActivationStrings {
213213

214214
export namespace UvInstallStrings {
215215
export const noPythonFound = l10n.t('No Python installation found');
216-
export const installPythonPrompt = l10n.t('No Python found. Would you like to install Python using uv?');
216+
export const installPythonPrompt = l10n.t(
217+
'No Python found. Would you like to install Python using uv? This will download and run an installer from https://astral.sh.',
218+
);
217219
export const installPythonAndUvPrompt = l10n.t(
218-
'No Python found. Would you like to install uv and use it to install Python?',
220+
'No Python found. Would you like to install uv and use it to install Python? This will download and run an installer from https://astral.sh.',
219221
);
220222
export const installPython = l10n.t('Install Python');
221223
export const installingUv = l10n.t('Installing uv...');

‎src/managers/builtin/sysPythonManager.ts‎

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -86,6 +86,8 @@ export class SysPythonManager implements EnvironmentManager {
8686
);
8787
if (resolved) {
8888
this.collection.push(resolved);
89+
this.globalEnv = resolved;
90+
await setSystemEnvForGlobal(resolved.environmentPath.fsPath);
8991
this._onDidChangeEnvironments.fire([
9092
{ environment: resolved, kind: EnvironmentChangeKind.add },
9193
]);
@@ -263,8 +265,10 @@ export class SysPythonManager implements EnvironmentManager {
263265
// Resolve the installed Python using NativePythonFinder instead of full refresh
264266
const resolved = await resolveSystemPythonEnvironmentPath(pythonPath, this.nativeFinder, this.api, this);
265267
if (resolved) {
266-
// Add to collection and fire change event
268+
// Add to collection, update global env, and fire change event
267269
this.collection.push(resolved);
270+
this.globalEnv = resolved;
271+
await setSystemEnvForGlobal(resolved.environmentPath.fsPath);
268272
this._onDidChangeEnvironments.fire([{ environment: resolved, kind: EnvironmentChangeKind.add }]);
269273
return resolved;
270274
}

‎src/managers/builtin/uvPythonInstaller.ts‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -348,6 +348,7 @@ export async function promptInstallPythonViaUv(
348348

349349
const result = await showInformationMessage(
350350
promptMessage,
351+
{ modal: true },
351352
UvInstallStrings.installPython,
352353
UvInstallStrings.dontAskAgain,
353354
);
@@ -414,10 +415,10 @@ export async function installPythonWithUv(log?: LogOutputChannel, version?: stri
414415
return undefined;
415416
}
416417

417-
// Step 3: Get the installed Python path using uv python find
418+
// Step 3: Get the installed Python path using uv-managed Python listing
418419
const pythonPath = await getUvPythonPath(version);
419420
if (!pythonPath) {
420-
traceError('Python installed but could not find the path via uv python find');
421+
traceError('Python installed but could not find the path via uv python list');
421422
sendTelemetryEvent(EventNames.UV_PYTHON_INSTALL_FAILED, undefined, { stage: 'findPath' });
422423
showErrorMessage(UvInstallStrings.installFailed);
423424
return undefined;

‎src/test/features/views/treeViewItems.unit.test.ts‎

Lines changed: 86 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,8 +1,10 @@
11
import * as assert from 'assert';
22
import { Uri } from 'vscode';
3+
import { UvInstallStrings, VenvManagerStrings } from '../../../common/localize';
34
import {
45
EnvManagerTreeItem,
56
getEnvironmentParentDirName,
7+
NoPythonEnvTreeItem,
68
PythonEnvTreeItem,
79
PythonGroupEnvTreeItem,
810
} from '../../../features/views/treeViewItems';
@@ -401,4 +403,88 @@ suite('Test TreeView Items', () => {
401403
assert.strictEqual(result, 'myapp');
402404
});
403405
});
406+
407+
suite('NoPythonEnvTreeItem', () => {
408+
test('System manager with create: shows install Python label', () => {
409+
const manager = new InternalEnvironmentManager('ms-python.python:test-manager', {
410+
name: 'system',
411+
displayName: 'Global',
412+
description: 'test',
413+
preferredPackageManagerId: 'pip',
414+
refresh: () => Promise.resolve(),
415+
getEnvironments: () => Promise.resolve([]),
416+
resolve: () => Promise.resolve(undefined),
417+
set: () => Promise.resolve(),
418+
get: () => Promise.resolve(undefined),
419+
create: () => Promise.resolve(undefined),
420+
});
421+
const managerItem = new EnvManagerTreeItem(manager);
422+
const item = new NoPythonEnvTreeItem(managerItem);
423+
424+
assert.equal(item.treeItem.label, UvInstallStrings.clickToInstallPython);
425+
assert.ok(item.treeItem.command, 'Should have a command');
426+
assert.equal(item.treeItem.command?.title, UvInstallStrings.installPython);
427+
assert.equal(item.treeItem.command?.command, 'python-envs.create');
428+
});
429+
430+
test('Non-system manager with create: shows create environment label', () => {
431+
const manager = new InternalEnvironmentManager('ms-python.python:test-manager', {
432+
name: 'venv',
433+
displayName: 'Venv',
434+
description: 'test',
435+
preferredPackageManagerId: 'pip',
436+
refresh: () => Promise.resolve(),
437+
getEnvironments: () => Promise.resolve([]),
438+
resolve: () => Promise.resolve(undefined),
439+
set: () => Promise.resolve(),
440+
get: () => Promise.resolve(undefined),
441+
create: () => Promise.resolve(undefined),
442+
});
443+
const managerItem = new EnvManagerTreeItem(manager);
444+
const item = new NoPythonEnvTreeItem(managerItem);
445+
446+
assert.equal(item.treeItem.label, VenvManagerStrings.noEnvClickToCreate);
447+
assert.ok(item.treeItem.command, 'Should have a command');
448+
assert.equal(item.treeItem.command?.title, VenvManagerStrings.createEnvironment);
449+
assert.equal(item.treeItem.command?.command, 'python-envs.create');
450+
});
451+
452+
test('Manager without create: shows no env found label', () => {
453+
const manager = new InternalEnvironmentManager('ms-python.python:test-manager', {
454+
name: 'test',
455+
displayName: 'Test',
456+
description: 'test',
457+
preferredPackageManagerId: 'pip',
458+
refresh: () => Promise.resolve(),
459+
getEnvironments: () => Promise.resolve([]),
460+
resolve: () => Promise.resolve(undefined),
461+
set: () => Promise.resolve(),
462+
get: () => Promise.resolve(undefined),
463+
});
464+
const managerItem = new EnvManagerTreeItem(manager);
465+
const item = new NoPythonEnvTreeItem(managerItem);
466+
467+
assert.equal(item.treeItem.label, VenvManagerStrings.noEnvFound);
468+
assert.equal(item.treeItem.command, undefined, 'Should not have a command');
469+
});
470+
471+
test('System manager without create: shows no env found label', () => {
472+
const manager = new InternalEnvironmentManager('ms-python.python:test-manager', {
473+
name: 'system',
474+
displayName: 'Global',
475+
description: 'test',
476+
preferredPackageManagerId: 'pip',
477+
refresh: () => Promise.resolve(),
478+
getEnvironments: () => Promise.resolve([]),
479+
resolve: () => Promise.resolve(undefined),
480+
set: () => Promise.resolve(),
481+
get: () => Promise.resolve(undefined),
482+
});
483+
const managerItem = new EnvManagerTreeItem(manager);
484+
const item = new NoPythonEnvTreeItem(managerItem);
485+
486+
assert.equal(item.treeItem.label, VenvManagerStrings.noEnvFound);
487+
assert.equal(item.treeItem.command, undefined, 'Should not have a command');
488+
});
489+
});
404490
});

‎src/test/managers/builtin/uvPythonInstaller.unit.test.ts‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -60,6 +60,7 @@ suite('uvPythonInstaller - promptInstallPythonViaUv', () => {
6060
assert(
6161
showInformationMessageStub.calledWith(
6262
UvInstallStrings.installPythonPrompt,
63+
{ modal: true },
6364
UvInstallStrings.installPython,
6465
UvInstallStrings.dontAskAgain,
6566
),
@@ -77,6 +78,7 @@ suite('uvPythonInstaller - promptInstallPythonViaUv', () => {
7778
assert(
7879
showInformationMessageStub.calledWith(
7980
UvInstallStrings.installPythonAndUvPrompt,
81+
{ modal: true },
8082
UvInstallStrings.installPython,
8183
UvInstallStrings.dontAskAgain,
8284
),

‎src/test/mocks/vsc/extHostedTypes.ts‎

Lines changed: 9 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1615,6 +1615,8 @@ export class ProcessExecution implements vscode.ProcessExecution {
16151615
export class ShellExecution implements vscode.ShellExecution {
16161616
private static idCounter = 0;
16171617

1618+
private _cachedId: string | undefined;
1619+
16181620
private _commandLine = '';
16191621

16201622
private _command: string | vscode.ShellQuotedString = '';
@@ -1708,10 +1710,13 @@ export class ShellExecution implements vscode.ShellExecution {
17081710
// }
17091711
// }
17101712
// return hash.digest('hex');
1711-
// Return a deterministic unique ID based on command and a monotonic counter
1712-
const cmd = typeof this._command === 'string' ? this._command : (this._command?.value ?? '');
1713-
const argsStr = this._args?.map((a) => (typeof a === 'string' ? a : a.value)).join(',') ?? '';
1714-
return `shell-${cmd}-${argsStr}-${ShellExecution.idCounter++}`;
1713+
// Memoize the computed ID per instance to ensure consistent task matching
1714+
if (this._cachedId === undefined) {
1715+
const cmd = typeof this._command === 'string' ? this._command : (this._command?.value ?? '');
1716+
const argsStr = this._args?.map((a) => (typeof a === 'string' ? a : a.value)).join(',') ?? '';
1717+
this._cachedId = `shell-${cmd}-${argsStr}-${ShellExecution.idCounter++}`;
1718+
}
1719+
return this._cachedId;
17151720
}
17161721
}
17171722

0 commit comments

Comments
 (0)