Skip to content

Commit 2157f65

Browse files
committed
Fix package manager review suggestions
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 5aba47cf-0b43-48b9-bc22-75e625da70e8
1 parent b54fcd9 commit 2157f65

4 files changed

Lines changed: 64 additions & 11 deletions

File tree

‎src/managers/builtin/pipPackageManager.ts‎

Lines changed: 8 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -145,8 +145,9 @@ export class PipPackageManager implements PackageManager, Disposable {
145145
this.log.error('Error managing packages', e);
146146
if (!options.runHeadless) {
147147
setImmediate(async () => {
148-
const result = await showErrorMessage('Error managing packages', 'View Output');
149-
if (result === 'View Output') {
148+
const viewOutput = l10n.t('View Output');
149+
const result = await showErrorMessage(l10n.t('Error managing packages'), viewOutput);
150+
if (result === viewOutput) {
150151
this.log.show();
151152
}
152153
});
@@ -178,7 +179,7 @@ export class PipPackageManager implements PackageManager, Disposable {
178179
await withProgress(
179180
{
180181
location: ProgressLocation.Window,
181-
title: 'Refreshing packages',
182+
title: l10n.t('Refreshing packages'),
182183
},
183184
async () => {
184185
const packages = await updatePackagesAndNotify(
@@ -226,9 +227,10 @@ export class PipPackageManager implements PackageManager, Disposable {
226227
this.log.error('Error refreshing packages', error);
227228
if (showErrors) {
228229
setImmediate(async () => {
229-
const viewOutput = l10n.t('View Output');
230+
const viewOutput = l10n.t('View Output');
230231
const result = await showErrorMessage(l10n.t('Error refreshing packages'), viewOutput);
231232
if (result === viewOutput) {
233+
this.log.show();
232234
}
233235
});
234236
}
@@ -282,9 +284,9 @@ const viewOutput = l10n.t('View Output');
282284

283285
// For pip < 21.2.0, check version first.
284286
if (availableVersionsCmd instanceof PipAvailableVersionsCommand) {
285-
const pipVersionCmd = new PipVersionCommand({ pythonExecutable, log: this.log });
286-
const pipVersion = await pipVersionCmd.execute();
287+
const pipVersion = await new PipVersionCommand({ pythonExecutable, log: this.log }).execute();
287288
if (!pipVersion) {
289+
throw new Error(`Unable to determine pip version for environment: ${environment.envId.id}`);
288290
}
289291
if (compare(pipVersion.public, '21.2.0') < 0) {
290292
throw new PackageVersionLookupNotSupportedError(

‎src/managers/builtin/pipUtils.ts‎

Lines changed: 1 addition & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -286,10 +286,7 @@ export async function getWorkspacePackagesToInstall(
286286
PipListCommand,
287287
UvListCommand,
288288
);
289-
const data = await withProgress(
290-
{ location: ProgressLocation.Notification },
291-
() => listCmd.execute(),
292-
);
289+
const data = await withProgress({ location: ProgressLocation.Notification }, () => listCmd.execute());
293290
installed = data.map((pkg) => pkg.name);
294291
} catch (error) {
295292
log?.error('Error listing installed packages', error);

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

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -95,6 +95,31 @@ suite('PipPackageManager', () => {
9595
);
9696
});
9797

98+
test('propagates pip version command failures during version lookup', async () => {
99+
const manager = createManager();
100+
const environment = createEnvironment();
101+
const versionError = new Error('pip version failed');
102+
sinon.stub(helpers, 'shouldUseUv').resolves(false);
103+
sinon.stub(helpers, 'runPython').rejects(versionError);
104+
105+
await assert.rejects(
106+
manager.getPackageAvailableVersions(environment, 'requests'),
107+
(error: unknown) => error === versionError,
108+
);
109+
});
110+
111+
test('rejects version lookup when pip version output cannot be parsed', async () => {
112+
const manager = createManager();
113+
const environment = createEnvironment();
114+
sinon.stub(helpers, 'shouldUseUv').resolves(false);
115+
sinon.stub(helpers, 'runPython').resolves('unexpected version output');
116+
117+
await assert.rejects(
118+
manager.getPackageAvailableVersions(environment, 'requests'),
119+
/Unable to determine pip version for environment: test-environment/,
120+
);
121+
});
122+
98123
test('normalizes discovered Python versions for pip lookup', async () => {
99124
const manager = createManager();
100125
const environment = {

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

Lines changed: 30 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
11
import assert from 'assert';
22
import * as path from 'path';
33
import * as sinon from 'sinon';
4-
import { CancellationToken, LogOutputChannel, Progress, ProgressOptions, Uri } from 'vscode';
4+
import { CancellationToken, LogOutputChannel, Progress, ProgressLocation, ProgressOptions, Uri } from 'vscode';
55
import * as fse from 'fs-extra';
66
import * as os from 'os';
77
import { PythonEnvironment, PythonEnvironmentApi, PythonProject } from '../../../api';
@@ -87,6 +87,35 @@ suite('Pip Utils - getProjectInstallable', () => {
8787
assert.ok(logError.calledOnceWithExactly('Error listing installed packages', listError));
8888
assert.ok(showQuickPick.calledOnce, 'The package picker should still open after a list failure');
8989
});
90+
91+
test('shows progress while listing installed packages', async () => {
92+
const environment = {
93+
environmentPath: Uri.file('.'),
94+
execInfo: { run: { executable: 'python' } },
95+
} as PythonEnvironment;
96+
const workspacePath = Uri.file('/test/path/root').fsPath;
97+
findFilesStub.callsFake((pattern: string) =>
98+
Promise.resolve(
99+
pattern === '*requirements*.txt'
100+
? [Uri.file(path.join(workspacePath, 'requirements.txt'))]
101+
: [],
102+
),
103+
);
104+
sinon.stub(helpers, 'shouldUseUv').resolves(false);
105+
sinon.stub(PipListCommand.prototype, 'execute').resolves([]);
106+
const showQuickPick = sinon.stub(winapi, 'showQuickPickWithButtons').resolves(undefined);
107+
108+
await getWorkspacePackagesToInstall(
109+
mockApi as PythonEnvironmentApi,
110+
{ install: [] },
111+
[{ name: 'workspace', uri: Uri.file(workspacePath) }],
112+
environment,
113+
);
114+
115+
assert.strictEqual(withProgressStub.callCount, 2);
116+
assert.strictEqual(withProgressStub.secondCall.args[0].location, ProgressLocation.Notification);
117+
assert.ok(withProgressStub.secondCall.calledBefore(showQuickPick.firstCall));
118+
});
90119
});
91120

92121
teardown(() => {

0 commit comments

Comments
 (0)