#2026: Create UvRepository and UvBasedCommandlet#2064
Conversation
|
|
Coverage Report for CI Build 28428343995Coverage decreased (-0.1%) to 71.226%Details
Uncovered ChangesNo uncovered changes found. Coverage Regressions188 previously-covered lines in 4 files lost coverage.
Coverage Stats
💛 - Coveralls |
quando632
left a comment
quando632
left a comment
There was a problem hiding this comment.
Clean implementation that follows the existing NpmRepository / PipRepository patterns well. Wiring, registration of Just, help texts and license entry are all complete, and Just is nicely minimal. Tests pass (JustTest, UvTest). I only have a few points.
| request.setProcessContext(pc); | ||
| ProcessResult result = runPackageManager(request); | ||
| if (result.isSuccessful()) { | ||
| String prefix = packageName + "v"; |
There was a problem hiding this comment.
Wrong prefix when parsing uv tool list
UvBasedCommandlet.java:79
String prefix = packageName + "v";The real uv tool list output is rust-just v1.37.0 (with a space), so this prefix never matches. As a result the installed version is never detected and getInstalledVersion() always returns null. Should be packageName + " v".
| .setProcessMode(ProcessMode.DEFAULT_CAPTURE); | ||
| ProcessContext pc = this.context.newProcess().errorHandling(ProcessErrorHandling.NONE); | ||
| request.setProcessContext(pc); | ||
| ProcessResult result = runPackageManager(request); |
There was a problem hiding this comment.
Version check can trigger an installation
computeInstalledVersion calls the one-arg runPackageManager(request). The base class @implNote explicitly says to use runPackageManager(request, true) with skipInstallation=true for version checks, since the one-arg form calls pm.install(...) internally.
There was a problem hiding this comment.
Missing validation
UvArtifact is missing the requireNotEmpty(name, ...) validation that PipArtifact has. Since getPackageName() currently always returns a hardcoded string this is not an immediate problem, but future UvBasedCommandlet implementations returning null or empty would only fail at the HTTP request rather than immediately with a clear exception.
There was a problem hiding this comment.
Tests don't cover computeInstalledVersion
The uv mock script only handles pip/venv and echoes uv tool list for the list call, so it never returns a real list. That's why the busg in UvBasedCommandlet went unnoticed. Adding a test with a realistic uv tool list output (e.g. rust-just v1.37.0) would cover both.
4 participants
This PR fixes #2026
Implemented changes
UvRepository(resolves tool versions from PyPI, reusing the existingPypiObjectJSON model) together withUvArtifactandUvArtifactMetadata.UvBasedCommandlet, the abstract base for tools installed viauv tool install.getUvRepository()onIdeContext/AbstractIdeContext.Justas the first concrete uv-based tool (PyPI packagerust-just, commandjust) and registered it inCommandletManagerImpl.UV_TOOL_BIN_DIRonto the path inUv.setEnvironment(viawithPathEntry()) so uv-installed tools are actually runnable.Testing instructions
list-versions just: lists rust-just versions from PyPI (newest first).install just: installs the latest version.install just 1.51.0: installs a specific version.just --version: runs the installed tool.mvn clean test(seeJustTest).Checklist for this PR
mvn clean testlocally all tests pass and build is successful#«issue-id»: «brief summary»In Progressand assigned to youinternalChecklist for tool commandlets
justJUST_VERSIONandJUST_EDITIONare honored by the commandlet