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

uninstall usability #121

Closed
pavelloz opened this issue Jul 4, 2019 · 6 comments · Fixed by #191
Closed

uninstall usability #121

pavelloz opened this issue Jul 4, 2019 · 6 comments · Fixed by #191

Comments

@pavelloz
Copy link
Contributor

pavelloz commented Jul 4, 2019

I can fnm install 12, but i cannot fnm uninstall 10 even if there is clearly only one fitting match.

Additionally, i have to type vXX.XX.X, numbers are not enough.

I think behavior could be more user friendly (which in this case also means consistent across commands).

marketplace-nearme-example|master⚡ ⇒ fnm ls
The following versions are installed:
* system
* v10.15.3
* v12.2.0
* v12.3.0
* v12.5.0
* v12.6.0 (default)
marketplace-nearme-example|master⚡ ⇒ fnm uninstall 10 
The version 10 is not installed.
marketplace-nearme-example|master⚡ ⇒ fnm uninstall 10.15.3
The version 10.15.3 is not installed.
marketplace-nearme-example|master⚡ ⇒ fnm uninstall v10.15.3
fnm: internal error, uncaught exception:
     Unix.Unix_error(Unix.ENOENT, "stat", "/Users/pavel/.fnm/node-versions/v10.15.3/installation/lib/node_modules/serverless/node_modules/.bin/flat")

PS. Ill report bug about error above in different issue - it looks like it didnt uninstall 10 because of that.

@Schniz
Copy link
Owner

Schniz commented Jul 4, 2019

The error shouldn't happen but I'm not quite sure if we really want to expand this. It can lead to unwanted uninstallations, which are destructive! Maybe we should print something like:

Did you meant to uninstall v10.15.3? Try again with:
> fnm uninstall v10.15.3

@Schniz
Copy link
Owner

Schniz commented Jul 4, 2019

what do you think? :)

@pavelloz
Copy link
Contributor Author

pavelloz commented Jul 4, 2019

Hmm, I see two things that can be improved:

  1. if there is only one match. Taking my case as an example: there is only one 10.x, so writing uninstall 10 we can assume the user wants to uninstall this one version that starts with 10.x.
    If more than one matches, I think the current behavior is correct - in that case guessing is not a good idea for the reason you brought up.
  2. Lack of consistency with v between install and uninstall is weird, it is visible the best when running one after another.
    marketplace-nearme-example|master⚡ ⇒ fnm install 12.6.0
    Version v12.6.0 was successfuly downloaded 
    marketplace-nearme-example|master⚡ ⇒ fnm uninstall 12.6.0
    The version 12.6.0 is not installed.
    

@Schniz
Copy link
Owner

Schniz commented Jul 7, 2019

Lack of consistency with v between install and uninstall is weird, it is visible the best when running one after another.

You're totally right! just made #125 that fixes that 😄

if there is only one match. Taking my case as an example: there is only one 10.x, so writing uninstall 10 we can assume the user wants to uninstall this one version that starts with 10.x.

this is interesting too. I hope I have time to do that!

@pavelloz
Copy link
Contributor Author

pavelloz commented Jul 7, 2019

Glad i could help - wish i could help with code, but ReasonML is so far out of my comfort zone it would be a disaster. Meanwhile, i can try to help however i can within my competence as a user :)

@waldyrious
Copy link
Contributor

waldyrious commented Jan 15, 2020

The error shouldn't happen but I'm not quite sure if we really want to expand this. It can lead to unwanted uninstallations, which are destructive! Maybe we should print something like:

Did you meant to uninstall v10.15.3? Try again with:
> fnm uninstall v10.15.3

If there's a single match, I'd prefer a simple [y|N] prompt rather than having to type the command again. In particular, I should be able to do fnm uninstall 10 after doing fnm install 10.

If there are multiple matching versions, I agree that showing a list of available versions would be the appropriate behavior.

Schniz pushed a commit that referenced this issue Feb 25, 2020
* If no version is matching => we print out a message (like we used to)
* If only one version is matching (because the user provided a specific version `6.17.1` or because the prefix match only one installed version) => the version get uninstalled
* If multiple installed versions are matched: we print out the matching installed versions and ask the user to re-run the command with the correct version to uninstall.

Note: it's not possible to uninstall using `latest` or `latest-v9.x`, ... But that's already the case so nothing has changed here.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants