Skip to content

Commit 16fe13e

Browse files
karthiknadigkarthiknadig
committed
Address PR review comments: modal prompt, memoize computeId, update globalEnv, fix comments, add tests
1 parent 49de5f8 commit 16fe13e

File tree

6 files changed

+109
-10
lines changed

6 files changed

+109
-10
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
@@ -85,6 +85,8 @@ export class SysPythonManager implements EnvironmentManager {
8585
);
8686
if (resolved) {
8787
this.collection.push(resolved);
88+
this.globalEnv = resolved;
89+
await setSystemEnvForGlobal(resolved.environmentPath.fsPath);
8890
this._onDidChangeEnvironments.fire([{ environment: resolved, kind: EnvironmentChangeKind.add }]);
8991
}
9092
}
@@ -259,8 +261,10 @@ export class SysPythonManager implements EnvironmentManager {
259261
// Resolve the installed Python using NativePythonFinder instead of full refresh
260262
const resolved = await resolveSystemPythonEnvironmentPath(pythonPath, this.nativeFinder, this.api, this);
261263
if (resolved) {
262-
// Add to collection and fire change event
264+
// Add to collection, update global env, and fire change event
263265
this.collection.push(resolved);
266+
this.globalEnv = resolved;
267+
await setSystemEnvForGlobal(resolved.environmentPath.fsPath);
264268
this._onDidChangeEnvironments.fire([{ environment: resolved, kind: EnvironmentChangeKind.add }]);
265269
return resolved;
266270
}

‎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 & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,8 @@
11
import * as assert from 'assert';
2-
import { EnvManagerTreeItem, PythonEnvTreeItem } from '../../../features/views/treeViewItems';
2+
import { EnvManagerTreeItem, NoPythonEnvTreeItem, PythonEnvTreeItem } from '../../../features/views/treeViewItems';
33
import { InternalEnvironmentManager, PythonEnvironmentImpl } from '../../../internal.api';
44
import { Uri } from 'vscode';
5+
import { UvInstallStrings, VenvManagerStrings } from '../../../common/localize';
56

67
suite('Test TreeView Items', () => {
78
suite('EnvManagerTreeItem', () => {
@@ -238,4 +239,88 @@ suite('Test TreeView Items', () => {
238239
assert.equal(item.treeItem.label, env.displayName);
239240
});
240241
});
242+
243+
suite('NoPythonEnvTreeItem', () => {
244+
test('System manager with create: shows install Python label', () => {
245+
const manager = new InternalEnvironmentManager('ms-python.python:test-manager', {
246+
name: 'system',
247+
displayName: 'Global',
248+
description: 'test',
249+
preferredPackageManagerId: 'pip',
250+
refresh: () => Promise.resolve(),
251+
getEnvironments: () => Promise.resolve([]),
252+
resolve: () => Promise.resolve(undefined),
253+
set: () => Promise.resolve(),
254+
get: () => Promise.resolve(undefined),
255+
create: () => Promise.resolve(undefined),
256+
});
257+
const managerItem = new EnvManagerTreeItem(manager);
258+
const item = new NoPythonEnvTreeItem(managerItem);
259+
260+
assert.equal(item.treeItem.label, UvInstallStrings.clickToInstallPython);
261+
assert.ok(item.treeItem.command, 'Should have a command');
262+
assert.equal(item.treeItem.command?.title, UvInstallStrings.installPython);
263+
assert.equal(item.treeItem.command?.command, 'python-envs.create');
264+
});
265+
266+
test('Non-system manager with create: shows create environment label', () => {
267+
const manager = new InternalEnvironmentManager('ms-python.python:test-manager', {
268+
name: 'venv',
269+
displayName: 'Venv',
270+
description: 'test',
271+
preferredPackageManagerId: 'pip',
272+
refresh: () => Promise.resolve(),
273+
getEnvironments: () => Promise.resolve([]),
274+
resolve: () => Promise.resolve(undefined),
275+
set: () => Promise.resolve(),
276+
get: () => Promise.resolve(undefined),
277+
create: () => Promise.resolve(undefined),
278+
});
279+
const managerItem = new EnvManagerTreeItem(manager);
280+
const item = new NoPythonEnvTreeItem(managerItem);
281+
282+
assert.equal(item.treeItem.label, VenvManagerStrings.noEnvClickToCreate);
283+
assert.ok(item.treeItem.command, 'Should have a command');
284+
assert.equal(item.treeItem.command?.title, VenvManagerStrings.createEnvironment);
285+
assert.equal(item.treeItem.command?.command, 'python-envs.create');
286+
});
287+
288+
test('Manager without create: shows no env found label', () => {
289+
const manager = new InternalEnvironmentManager('ms-python.python:test-manager', {
290+
name: 'test',
291+
displayName: 'Test',
292+
description: 'test',
293+
preferredPackageManagerId: 'pip',
294+
refresh: () => Promise.resolve(),
295+
getEnvironments: () => Promise.resolve([]),
296+
resolve: () => Promise.resolve(undefined),
297+
set: () => Promise.resolve(),
298+
get: () => Promise.resolve(undefined),
299+
});
300+
const managerItem = new EnvManagerTreeItem(manager);
301+
const item = new NoPythonEnvTreeItem(managerItem);
302+
303+
assert.equal(item.treeItem.label, VenvManagerStrings.noEnvFound);
304+
assert.equal(item.treeItem.command, undefined, 'Should not have a command');
305+
});
306+
307+
test('System manager without create: shows no env found label', () => {
308+
const manager = new InternalEnvironmentManager('ms-python.python:test-manager', {
309+
name: 'system',
310+
displayName: 'Global',
311+
description: 'test',
312+
preferredPackageManagerId: 'pip',
313+
refresh: () => Promise.resolve(),
314+
getEnvironments: () => Promise.resolve([]),
315+
resolve: () => Promise.resolve(undefined),
316+
set: () => Promise.resolve(),
317+
get: () => Promise.resolve(undefined),
318+
});
319+
const managerItem = new EnvManagerTreeItem(manager);
320+
const item = new NoPythonEnvTreeItem(managerItem);
321+
322+
assert.equal(item.treeItem.label, VenvManagerStrings.noEnvFound);
323+
assert.equal(item.treeItem.command, undefined, 'Should not have a command');
324+
});
325+
});
241326
});

‎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)