Adopt package manager command classes - #1686
Adopt package manager command classes#1686Eduardo Villalpando Mello (edvilme) wants to merge 13 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
This PR migrates Pip/UV, Conda, and Poetry package manager call sites to the newer package-manager command class layer (introduced in #1621), and removes older helper utilities/tests that those call sites depended on.
Changes:
- Replaces ad-hoc command construction/execution with concrete command classes for Pip/UV, Conda, and Poetry.
- Refactors Pip/Conda/Poetry package manager flows to use command objects (including progress + cancellation wiring).
- Removes superseded parsing/execution utilities and replaces related unit tests with command-focused tests.
Reviewed changes
Copilot reviewed 13 out of 14 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| src/test/managers/poetry/commands.unit.test.ts | Adds Poetry command smoke tests (currently mostly “does not reject”). |
| src/test/managers/conda/commands.unit.test.ts | Adds Conda command smoke tests (currently mostly “does not reject”). |
| src/test/managers/builtin/commands.unit.test.ts | Adds Pip/UV command smoke tests (currently mostly “does not reject”). |
| src/test/managers/builtin/pipVersions.unit.test.ts | Removes tests for the old pip-index-versions parsing helper. |
| src/test/managers/builtin/pipListUtils.unit.test.ts | Removes tests for the old pip list / uv tree parsing helpers. |
| src/managers/poetry/poetryPackageManager.ts | Switches Poetry package operations to command classes; refactors Poetry cwd selection into helper. |
| src/managers/conda/condaUtils.ts | Removes superseded managePackages() helper. |
| src/managers/conda/condaPackageManager.ts | Switches Conda package operations/list/version/available-versions to command classes. |
| src/managers/builtin/utils.ts | Adds parsePackageSpecs() helper and removes older pip-related utility functions from this module. |
| src/managers/builtin/pipUtils.ts | Switches installed-package enumeration to command classes; removes isPipInstallCommand(). |
| src/managers/builtin/pipPackageManager.ts | Switches pip/uv operations to command classes; refactors available versions and direct-name listing. |
| src/managers/builtin/pipListUtils.ts | Deletes the old pip list / uv tree parsing module. |
| src/managers/builtin/helpers.ts | Adjusts logging behavior for python stderr output. |
| package-lock.json | Lockfile update (tslib metadata change). |
- pipPackageManager.manage(): rethrow CancellationError instead of swallowing it, so callers (e.g. venv creation's pkgInstallationCancelled) can distinguish cancel from failure; restores parity with main and the conda/poetry managers. - getDirectPackageNames: fix docstring to reflect uv uses 'uv pip tree --depth=0' (not 'uv pip list --not-required'). - parsePackageSpecs: omit undefined version property and clarify wording. - runPython: restore 'python:' prefix on stderr log output for consistency. - Remove overlapping command smoke tests; the reworked, parsed-value versions are owned by the tests PR (#1677). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 3f16397e-0917-4efb-8d75-566c71ebf9ba
Public-API-driven integration test exercising install/list/direct-deps/uninstall for every discovered package manager, parametrized over environments grouped by managerId so future managers are covered automatically. Consolidated into this PR per review preference rather than a separate PR. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 3f16397e-0917-4efb-8d75-566c71ebf9ba
Add a capability-guarded available-versions step to the package manager roundtrip integration test. Uses an optional cast so it compiles and runs regardless of whether the active API build surfaces the getter, and skips the assertion for managers that resolve to undefined. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 3f16397e-0917-4efb-8d75-566c71ebf9ba
The package-manager roundtrip hard-failed for conda on CI because cowsay is not available on conda's default channels and conda surfaces install failures as notifications rather than throwing, so the post-install assertion tripped. Make the install step best-effort: verify presence after install and, when the package did not install from the manager's configured sources, skip that manager gracefully instead of failing. Managers that can install it still assert the full lifecycle. Also restore package-lock.json to match main (spurious tslib 'peer' metadata churn from a local npm install; no package.json change) to satisfy the changed-files check. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 3f16397e-0917-4efb-8d75-566c71ebf9ba
…ndtrip
Switch the roundtrip integration test package from cowsay to flask.
cowsay is not on conda's default channels, so conda's managePackages
silently swallowed the failed install and the post-install assertion
tripped on CI. flask is available across all managers (incl. conda's
default main channel) and pulls in transitive deps (jinja2, werkzeug,
click, ...), letting the test verify direct-vs-transitive classification.
Install is a hard assertion again (no best-effort skip). The enriched
package list is read from refreshPackages' return value, since
getPackages({skipCache}) re-lists without isTransitive enrichment. The
transitive-dependency assertion is conditional on the manager classifying
transitivity at all (conda leaves isTransitive undefined). Bump per-test
timeout to 5m to accommodate conda solving flask + deps.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 3f16397e-0917-4efb-8d75-566c71ebf9ba
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 10 changed files in this pull request and generated no new comments.
Suppressed comments (3)
src/managers/builtin/pipPackageManager.ts:185
getPackages()now directly awaitslistCmd.execute()and then caches the result. IfrunPython/runUVfails (e.g. pip missing/broken),execute()will reject and the public API call will throw rather than returningundefinedas before, potentially breaking callers/UI. Consider catching execution errors here and returningundefined(and avoid caching an empty list on failure).
const data = await listCmd.execute();
const packages = (data ?? []).map((pkg) => this.api.createPackageItem(pkg, environment, this));
this.packages.set(environment.envId.id, packages);
return packages;
src/managers/builtin/pipUtils.ts:289
getWorkspacePackagesToInstall()now callslistCmd.execute()without any error handling. If pip/uv listing fails, this helper will throw and can abort the package selection flow. Previously the pip list path handled failures and returnedundefined/empty results. Consider catching here and treating the installed list as unknown so the picker can still proceed.
UvListCommand,
);
const data = await listCmd.execute();
installed = data?.map((pkg) => pkg.name);
}
src/managers/poetry/poetryPackageManager.ts:234
fetchPackagesFromTool()will currently throw (e.g. if Poetry is missing orpoetry showfails), andPythonEnvironmentApi.getPackages()forwards manager errors without catching. To keepgetPackages()resilient for callers/UI, consider making this helper return an empty list on tool failure (and log), rather than throwing.
const cwd = await this.getPoetryCwd(environment);
const showCmd = new PoetryShowCommand({
pythonExecutable: poetry,
cwd,
log: this.log,
|
🔒 Automated review in progress — Stella Huang (@StellaHuang95) is auto-reviewing this PR. |
|
GitHub cannot anchor PR review comments to unchanged lines in the diff. Falling back to a general PR comment for src/test/managers/builtin/pipListUtils.unit.test.ts:L1.
Deleting these parser tests removes deterministic malformed-output coverage for pip list, uv tree, and available-version parsing. The new live integration test does not exercise those branches; retain equivalent command-layer unit coverage in this change or ensure it is already present before removing these tests. [verified] |
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 6b2fe9b5-38ea-442f-b07a-b6c71134d480
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 6b2fe9b5-38ea-442f-b07a-b6c71134d480
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 6b2fe9b5-38ea-442f-b07a-b6c71134d480
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 6b2fe9b5-38ea-442f-b07a-b6c71134d480
|
Stella Huang (@StellaHuang95) Addressed the parser-coverage recommendation in 39f8559. Added deterministic command-layer tests for pip/uv list parsing, malformed and non-array output, incomplete entries, uv tree top-level filtering, direct-name normalization, and available-version parsing/filtering. This retains coverage without restoring the superseded legacy parser helpers. |
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 6b2fe9b5-38ea-442f-b07a-b6c71134d480
Summary
Adopts the package manager command classes introduced in #1621 across Pip/UV, Conda, and Poetry.
Changes
managerId, so future managers are covered automatically. The available-versions step is capability-guarded so it runs when the API surfacesgetPackageAvailableVersionsand skips gracefully otherwise.Relationship
maindirectly and depends only on the already-merged command classes (Introduce package manager command classes #1621). Independent of the smoke-tests PR Add package manager command smoke tests #1677.main.Testing
npm run compile-testsnpm run unittest(1,475 passing)npm run integration-test.