Add error handling for broken Python environments#1189
Conversation
There was a problem hiding this comment.
Pull request overview
This PR implements error handling for broken Python environments by adding an error field to environment interfaces and updating the UI to display warnings and error messages for environments with issues (e.g., broken symlinks, missing executables). This allows users to see and diagnose broken environments that were previously silently skipped.
Changes:
- Added
error?: stringfield toNativeEnvInfo,PythonEnvironmentInfo, andPythonEnvironmentImplinterfaces - Modified
getPythonInfo()to create environment items for broken environments with warning icons and error messages - Updated
findVirtualEnvironments()to include broken environments in the collection instead of skipping them - Enhanced tree view items to display warning icons and error messages for broken environments, with different context values to prevent normal actions (like activation/selection)
- Added package.json menu items to allow removing and copying paths for broken environments
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| src/managers/common/nativePythonFinder.ts | Added error?: string field to NativeEnvInfo interface with documentation |
| src/managers/builtin/venvUtils.ts | Added error handling in getPythonInfo() to create broken environment items and modified findVirtualEnvironments() to include them |
| src/internal.api.ts | Added error field to PythonEnvironmentImpl class constructor and property |
| src/features/views/treeViewItems.ts | Updated PythonEnvTreeItem and ProjectEnvironment to display warning icons and error messages, with special context values for broken environments |
| src/api.ts | Added error?: string field to PythonEnvironmentInfo interface and reformatted some interface declarations |
| package.json | Added menu items for remove and copy path commands for broken environments, plus whitespace fix |
Comments suppressed due to low confidence (1)
src/managers/builtin/venvUtils.ts:150
- The
executablefield inexecInfo.runis set toenv.executable ?? '', which could be an empty string. This is problematic because if code tries to execute this environment, it will fail. Consider whether broken environments should throw an error if someone attempts to execute them, or if the executable field should point to a non-existent but syntactically valid path that makes the error clearer.
execInfo: {
run: {
executable: env.executable ?? '',
},
| version: env.version ?? 'Unknown', | ||
| description: env.error, | ||
| tooltip: env.error, | ||
| environmentPath: Uri.file(env.prefix ?? env.executable ?? ''), |
There was a problem hiding this comment.
Critical cross-platform path handling issue: When env.prefix and env.executable are both undefined/empty, Uri.file('') is called which will fail on Windows. According to the coding guidelines, this extension must work on Windows, macOS, and Linux.
For broken environments, at least one of env.prefix, env.executable, or a fallback should be guaranteed to exist. If truly none are available, the environment should not be created or a safe fallback path should be used instead of an empty string.
| tooltip: env.error, | ||
| environmentPath: Uri.file(env.prefix ?? env.executable ?? ''), | ||
| iconPath: new ThemeIcon('warning'), | ||
| sysPrefix: env.prefix ?? '', |
There was a problem hiding this comment.
The sysPrefix field is being set to env.prefix ?? '', which means a broken environment could have an empty string as its sysPrefix. This could lead to issues if other code expects sysPrefix to be a valid directory path. Consider whether broken environments should set this to a valid path or if downstream code should handle empty sysPrefix appropriately.
| sysPrefix: env.prefix ?? '', | |
| sysPrefix: env.prefix ?? (env.executable ? path.dirname(env.executable) : ''), |
| // Handle broken environments that have an error field | ||
| if (env.error) { | ||
| const venvName = env.name ?? (env.prefix ? path.basename(env.prefix) : 'Unknown'); | ||
| const name = `${venvName} (broken)`; | ||
|
|
||
| return { | ||
| name: name, | ||
| displayName: name, | ||
| shortDisplayName: `(${venvName})`, | ||
| displayPath: env.prefix ?? env.executable ?? 'Unknown path', | ||
| version: env.version ?? 'Unknown', | ||
| description: env.error, | ||
| tooltip: env.error, | ||
| environmentPath: Uri.file(env.prefix ?? env.executable ?? ''), | ||
| iconPath: new ThemeIcon('warning'), | ||
| sysPrefix: env.prefix ?? '', | ||
| execInfo: { | ||
| run: { | ||
| executable: env.executable ?? '', | ||
| }, | ||
| }, | ||
| error: env.error, | ||
| }; | ||
| } |
There was a problem hiding this comment.
The new error handling logic for broken environments in getPythonInfo() and findVirtualEnvironments() lacks test coverage. The codebase has comprehensive test coverage for other parts of venvUtils (see venvUtils.removeVenv.unit.test.ts and venvUtils.uvTracking.unit.test.ts), so tests should be added for:
getPythonInfo()with an environment that has an error field setfindVirtualEnvironments()encountering a broken environment- Verifying the returned broken environment has the correct properties (warning icon, error message in description/tooltip, etc.)
| @@ -77,21 +82,26 @@ export class PythonEnvTreeItem implements EnvTreeItem { | |||
|
|
|||
| const item = new TreeItem(name, TreeItemCollapsibleState.Collapsed); | |||
| item.contextValue = this.getContextValue(); | |||
| item.description = environment.description; | |||
| item.tooltip = tooltip; | |||
| item.iconPath = environment.iconPath; | |||
| // Show error message for broken environments | |||
| item.description = isBroken ? environment.error : environment.description; | |||
| item.tooltip = isBroken ? environment.error : tooltip; | |||
| // Show warning icon for broken environments | |||
| item.iconPath = isBroken ? new ThemeIcon('warning') : environment.iconPath; | |||
| this.treeItem = item; | |||
| } | |||
There was a problem hiding this comment.
The new error handling logic in PythonEnvTreeItem and ProjectEnvironment for displaying broken environments lacks test coverage. The existing test suite (treeViewItems.unit.test.ts) tests context values, labels, and other properties for normal environments, but doesn't cover broken environments. Tests should be added to verify:
- When an environment has an
errorfield, the context value ispythonBrokenEnvironmentinstead ofpythonEnvironment - The description and tooltip show the error message
- The icon is a warning icon
- Activatable is not set for broken environments
| // Handle broken environments that have an error field | ||
| if (env.error) { | ||
| const venvName = env.name ?? (env.prefix ? path.basename(env.prefix) : 'Unknown'); | ||
| const name = `${venvName} (broken)`; |
There was a problem hiding this comment.
The user-facing string "(broken)" should be localized using the l10n API. According to the coding guidelines, all user-facing messages must be localized. Consider adding something like l10n.t('broken') in the localize.ts file and using it here instead of the hardcoded "(broken)" string. Also ensure "(broken)" is properly formatted with parentheses for localization (e.g., l10n.t('{0} (broken)', venvName) to handle languages that might format this differently).
2 participants
Fixes #1178
Implement error handling for broken Python environments, enhancing the user interface to display warnings and error messages. This allows users to diagnose and address issues more effectively.