Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Better error messages for unsupported uninstall arguments #1786

Merged
merged 2 commits into from
Jul 10, 2024

Conversation

chriskrycho
Copy link
Contributor

@chriskrycho chriskrycho commented Jul 9, 2024

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 '[email protected]' found to uninstall

Notice that the old message treated '[email protected]' 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 [email protected] as a package, but rather as a runtime and version specifier, and can fall into the normal handling for runtimes.

Background and Miscellanies.

  • I noticed this when triaging uninstall failed #1760.
  • I also got a bit lost trying to figure out what volta_uninstall vs. direct_uninstall meant, so I commented those.
  • Likewise with some typo fixes!

@chriskrycho chriskrycho force-pushed the chriskrycho/better-reporting branch from 24d3c78 to f2d7514 Compare July 9, 2024 22:01
@chriskrycho chriskrycho force-pushed the chriskrycho/better-reporting branch from f2d7514 to fda493e Compare July 9, 2024 22:12
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 '[email protected]' found to uninstall
```

Notice that the old message treated `'[email protected]'` 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 `[email protected]` as a package, but rather as a runtime and
version specifier, and can fall into the normal handling for runtimes.
Copy link
Contributor

@charlespierce charlespierce left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Love to have better / clearer error messages 💥

@charlespierce charlespierce merged commit e40704c into main Jul 10, 2024
11 checks passed
@charlespierce charlespierce deleted the chriskrycho/better-reporting branch July 10, 2024 03:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants