Add venv package manager support to packages pane#12029
Add venv package manager support to packages pane#12029bricestacey wants to merge 6 commits intomainfrom
Conversation
Adds explicit support for Python venv environments in the packages pane: - Create VenvPackageManager class extending PipPackageManager - Add PackageManagerFactory for environment-based manager selection - Update PythonRuntimeSession to use factory pattern - Change PipPackageManager methods from private to protected See #11662
|
E2E Tests 🚀 |
| * @returns The appropriate package manager for the environment | ||
| */ | ||
| static create( | ||
| runtimeSource: string, |
There was a problem hiding this comment.
Can you type this as EnvironmentType?
There was a problem hiding this comment.
The value from Positron is a string (runtime metadata).
I went ahead and did the union of the two EnvironmentType | string as its more expressive and type safe.
There was a problem hiding this comment.
I like the PackageManagerFactory pattern in this PR but I don't think the VenvPackageManager is necessary. I think it's important to keep the two distinct concepts of:
- Environment type
- Package manager
These are sometimes exclusively one-to-one, but often not. I'd consider pip a package manager and not an environment type. And I'd consider venv an environment type but not a package manager.
It should still be okay to do something like "if venv type, then use the pip manager" in the factory.
| this._interpreterPathService = serviceContainer.get<IInterpreterPathService>(IInterpreterPathService); | ||
| this._envVarsService = serviceContainer.get<IEnvironmentVariablesService>(IEnvironmentVariablesService); | ||
| this._pipPackageManager = new PipPackageManager(this._pythonPath, this._messageEmitter, serviceContainer); | ||
| this._pipPackageManager = PackageManagerFactory.create( |
There was a problem hiding this comment.
nit: rename this to _packageManager?
There was a problem hiding this comment.
Agreed that we should put this code in a subdirectory.
austin3dickey
left a comment
austin3dickey
left a comment
There was a problem hiding this comment.
Looks great, thank you!!
| * | ||
| * This factory examines the Python environment type and returns the corresponding | ||
| * package manager implementation: | ||
| * - Venv environments use VenvPackageManager |
There was a problem hiding this comment.
nit: this docstring is a bit out of date now
2 participants
Adds explicit support for Python venv environments in the packages pane. For clarity, venv is supported out of the box using the PipPackageManager, but this is explicit and sets us up for other package managers that would need wildly different implementations:
Some thoughts on code quality, welcome encouragement to do any of them or feedback --
See #11662
Release Notes
New Features
Bug Fixes
QA Notes
This "just works" for venv environments. In fact, none of these code changes are necessary. However, the Factory design pattern sets us up for adding other package managers.