Skip to content

Commit 0b57449

Browse files
committed
fix: address offline review comments
1 parent 2610f57 commit 0b57449

File tree

6 files changed

+41
-18
lines changed

6 files changed

+41
-18
lines changed

src/api.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -596,7 +596,7 @@ export interface PackageManager {
596596
/**
597597
* Installs/Uninstall packages in the specified Python environment.
598598
* @param environment - The Python environment in which to install packages.
599-
* @param packages - The packages to install.
599+
* @param options - Options for managing packages.
600600
* @returns A promise that resolves when the installation is complete.
601601
*/
602602
manage(environment: PythonEnvironment, options: PackageManagementOptions): Promise<void>;

src/common/telemetry/constants.ts

Lines changed: 4 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,7 @@ export enum EventNames {
88
VENV_USING_UV = 'VENV.USING_UV',
99
VENV_CREATION = 'VENV.CREATION',
1010

11-
PACKAGE_MANAGE = 'PACKAGE.MANAGE',
11+
PACKAGE_MANAGEMENT = 'PACKAGE_MANAGEMENT',
1212
}
1313

1414
// Map all events to their properties
@@ -59,13 +59,11 @@ export interface IEventNamePropertyMapping {
5959
/* __GDPR__
6060
"package.install": {
6161
"managerId": { "classification": "SystemMetaData", "purpose": "FeatureInsight", "owner": "karthiknadig" },
62-
"installPackageCount": { "classification": "SystemMetaData", "purpose": "FeatureInsight", "owner": "karthiknadig" },
63-
"uninstallPackageCount": { "classification": "SystemMetaData", "purpose": "FeatureInsight", "owner": "karthiknadig" }
62+
"result": { "classification": "SystemMetaData", "purpose": "FeatureInsight", "owner": "karthiknadig" }
6463
}
6564
*/
66-
[EventNames.PACKAGE_MANAGE]: {
65+
[EventNames.PACKAGE_MANAGEMENT]: {
6766
managerId: string;
68-
installPackageCount: number;
69-
uninstallPackageCount: number;
67+
result: 'success' | 'error' | 'cancelled';
7068
};
7169
}

src/extension.ts

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
11
import { commands, ExtensionContext, LogOutputChannel, Terminal, Uri } from 'vscode';
22

33
import { PythonEnvironmentManagers } from './features/envManagers';
4-
import { registerLogger, traceInfo } from './common/logging';
4+
import { registerLogger, traceError, traceInfo } from './common/logging';
55
import { EnvManagerView } from './features/views/envManagersView';
66
import {
77
addPythonProject,
@@ -138,7 +138,11 @@ export async function activate(context: ExtensionContext): Promise<PythonEnviron
138138
envManagers,
139139
projectManager,
140140
);
141-
packageManager.manage(environment, { install: [] });
141+
try {
142+
packageManager.manage(environment, { install: [] });
143+
} catch (err) {
144+
traceError('Error when running command python-envs.packages', err);
145+
}
142146
}),
143147
commands.registerCommand('python-envs.uninstallPackage', async (context: unknown) => {
144148
await handlePackageUninstall(context, envManagers);

src/internal.api.ts

Lines changed: 15 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
import { Disposable, Event, LogOutputChannel, MarkdownString, Uri } from 'vscode';
1+
import { CancellationError, Disposable, Event, LogOutputChannel, MarkdownString, Uri } from 'vscode';
22
import {
33
PythonEnvironment,
44
EnvironmentManager,
@@ -244,12 +244,20 @@ export class InternalPackageManager implements PackageManager {
244244
}
245245

246246
async manage(environment: PythonEnvironment, options: PackageManagementOptions): Promise<void> {
247-
await this.manager.manage(environment, options);
248-
sendTelemetryEvent(EventNames.PACKAGE_MANAGE, undefined, {
249-
managerId: this.id,
250-
installPackageCount: (options.install ?? []).length,
251-
uninstallPackageCount: (options.uninstall ?? []).length,
252-
});
247+
try {
248+
await this.manager.manage(environment, options);
249+
sendTelemetryEvent(EventNames.PACKAGE_MANAGEMENT, undefined, { managerId: this.id, result: 'success' });
250+
} catch (error) {
251+
if (error instanceof CancellationError) {
252+
sendTelemetryEvent(EventNames.PACKAGE_MANAGEMENT, undefined, {
253+
managerId: this.id,
254+
result: 'cancelled',
255+
});
256+
throw error;
257+
}
258+
sendTelemetryEvent(EventNames.PACKAGE_MANAGEMENT, undefined, { managerId: this.id, result: 'error' });
259+
throw error;
260+
}
253261
}
254262

255263
refresh(environment: PythonEnvironment): Promise<void> {

src/managers/builtin/pipManager.ts

Lines changed: 14 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,13 @@
1-
import { Event, EventEmitter, LogOutputChannel, MarkdownString, ProgressLocation, ThemeIcon, window } from 'vscode';
1+
import {
2+
CancellationError,
3+
Event,
4+
EventEmitter,
5+
LogOutputChannel,
6+
MarkdownString,
7+
ProgressLocation,
8+
ThemeIcon,
9+
window,
10+
} from 'vscode';
211
import {
312
DidChangePackagesEventArgs,
413
IconPath,
@@ -82,13 +91,17 @@ export class PipPackageManager implements PackageManager, Disposable {
8291
this.packages.set(environment.envId.id, after);
8392
this._onDidChangePackages.fire({ environment, manager: this, changes });
8493
} catch (e) {
94+
if (e instanceof CancellationError) {
95+
throw e;
96+
}
8597
this.log.error('Error managing packages', e);
8698
setImmediate(async () => {
8799
const result = await window.showErrorMessage('Error managing packages', 'View Output');
88100
if (result === 'View Output') {
89101
this.log.show();
90102
}
91103
});
104+
throw e;
92105
}
93106
},
94107
);

src/managers/conda/condaPackageManager.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -85,7 +85,7 @@ export class CondaPackageManager implements PackageManager, Disposable {
8585
this._onDidChangePackages.fire({ environment: environment, manager: this, changes });
8686
} catch (e) {
8787
if (e instanceof CancellationError) {
88-
return;
88+
throw e;
8989
}
9090

9191
this.log.error('Error installing packages', e);

0 commit comments

Comments
 (0)