Skip to content

Commit

Permalink
Better error messages for unsupported uninstall arguments
Browse files Browse the repository at this point in the history
Instead of parsing whatever string the user supplied as the tool name
and supplying a default `VersionSpec`, attempt to parse the value as a
full specifier which may include a version. This means we can provide
better errors when the user passes a version. Specifically we can report
that Volta does not yet support uninstalling specific versions of tools.
Previously, we would report something like this:

```
warning:  No package 'typescript@5.4' found to uninstall
```

Notice that the old message treated `'typescript@5.4'` as the name of
the tool, when it should have been treating it as a tool and a version
specifier. Now, we instead report that uninstalling specific versions of
tools is unsupported.

The original motivation here was noticing that we printed errors like
that if the user tried to uninstall a runtime or a package manager with
a version specifier. This fixes that as well, since it no longer parses
a string like `node@20.14.5` as a package, but rather as a runtime and
version specifier, and can fall into the normal handling for runtimes.
  • Loading branch information
chriskrycho committed Jul 9, 2024
1 parent fd8cc68 commit 66da382
Show file tree
Hide file tree
Showing 2 changed files with 28 additions and 11 deletions.
18 changes: 15 additions & 3 deletions src/command/uninstall.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
use volta_core::error::{ExitCode, Fallible};
use volta_core::error::{ErrorKind, ExitCode, Fallible};
use volta_core::session::{ActivityKind, Session};
use volta_core::tool;
use volta_core::version::VersionSpec;
Expand All @@ -15,8 +15,20 @@ impl Command for Uninstall {
fn run(self, session: &mut Session) -> Fallible<ExitCode> {
session.add_event_start(ActivityKind::Uninstall);

let version = VersionSpec::default();
let tool = tool::Spec::from_str_and_version(&self.tool, version);
let tool = tool::Spec::try_from_str(&self.tool)?;

// For packages, specifically report that we do not support uninstalling
// specific versions. For runtimes and package managers, we currently
// *intentionally* let this fall through to inform the user that we do
// not support uninstalling those *at all*.
if let tool::Spec::Package(_name, version) = &tool {
let VersionSpec::None = version else {
return Err(ErrorKind::Unimplemented {
feature: "uninstalling specific versions of tools".into(),
}
.into());
};
}

tool.uninstall()?;

Expand Down
21 changes: 13 additions & 8 deletions tests/acceptance/volta_uninstall.rs
Original file line number Diff line number Diff line change
Expand Up @@ -96,6 +96,8 @@ fn uninstall_package_basic() {
assert!(!Sandbox::package_image_exists("cowsay"));
}

// The setup here is the same as the above, but here we check to make sure that
// if the user supplies a version, we error correctly.
#[test]
fn uninstall_package_basic_with_version() {
// basic uninstall - everything exists, and everything except the cached
Expand All @@ -116,14 +118,6 @@ fn uninstall_package_basic_with_version() {
"[..]error: uninstalling specific versions of tools is not supported yet."
)
);

// check that nothing is deleted.
assert!(Sandbox::package_config_exists("cowsay"));
assert!(Sandbox::bin_config_exists("cowsay"));
assert!(Sandbox::bin_config_exists("cowthink"));
assert!(Sandbox::shim_exists("cowsay"));
assert!(Sandbox::shim_exists("cowthink"));
assert!(Sandbox::package_image_exists("cowsay"));
}

#[test]
Expand Down Expand Up @@ -210,3 +204,14 @@ fn uninstall_package_orphaned_bins() {
assert!(!Sandbox::shim_exists("cowsay"));
assert!(!Sandbox::shim_exists("cowthink"));
}

#[test]
fn uninstall_runtime() {
let s = sandbox().build();
assert_that!(
s.volta("uninstall node"),
execs()
.with_status(1)
.with_stderr_contains("[..]error: Uninstalling node is not supported yet.")
)
}

0 comments on commit 66da382

Please sign in to comment.