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

[Fix] nvm install: Handle 'N/A' version instead of asking to install it #1305

Merged
merged 1 commit into from
Nov 16, 2016

Conversation

PeterDaveHello
Copy link
Collaborator

@PeterDaveHello PeterDaveHello commented Nov 14, 2016

Fixes #1304.

@PeterDaveHello
Copy link
Collaborator Author

hmmmm ... still trying to find out the problem here ... 😭

@PeterDaveHello
Copy link
Collaborator Author

I have no idea about the failed of Running "nvm exec --lts" should work, why nvm install --lts should make nvm use 1.0.0 work?

@PeterDaveHello PeterDaveHello force-pushed the nvm-install-NA-fix branch 2 times, most recently from 539bbd4 to 8bdd38c Compare November 14, 2016 19:30
@PeterDaveHello PeterDaveHello changed the title [Fix] Handle 'N/A' version instead of asking to install it, fix #1304 [WIP] [Fix] Handle 'N/A' version instead of asking to install it, fix #1304 Nov 14, 2016
@ljharb
Copy link
Member

ljharb commented Nov 14, 2016

I'm not sure I understand the problem. If you nvm use X, and X isn't installed, you get "You need to run "nvm install N/A" to install it before using it." - certainly the N/A should say nvm install X instead, but that's a pretty minor text change.

@PeterDaveHello
Copy link
Collaborator Author

It's very strange to ask for installing N/A I think, this change could make it smarter:

$ nvm use 4.2
N/A: version "4.2 -> v4.2.6" is not yet installed.

You need to run "nvm install 4.2" to install it before using it.

$ nvm use 4.20
Version '4.20' not found - try `nvm ls-remote` to browse available versions.

@ljharb
Copy link
Member

ljharb commented Nov 15, 2016

I agree that a better error message is an improvement - but adding an nvm_remote_version call seems like overkill. I'd be fine with just using whatever they provided into nvm install in the error message.

@PeterDaveHello
Copy link
Collaborator Author

@ljharb do you mean just print nvm install $PROVIDED_VERSION ? I use nvm_remote_version because it can help to confirm if the $PROVIDED_VERSION is valid or not.

@ljharb
Copy link
Member

ljharb commented Nov 15, 2016

I think that's nvm install's job :-)

@PeterDaveHello
Copy link
Collaborator Author

PeterDaveHello commented Nov 15, 2016

@ljharb what about this?

      if [ "${VERSION}" = 'N/A' ]; then
        nvm_err "N/A: version \"${PROVIDED_VERSION} -> ${VERSION}\" is not yet installed."
        nvm_err ""
        nvm_err "You need to run \"nvm install ${PROVIDED_VERSION}\" to install it before using it."
        return 3
      # This nvm_ensure_version_installed call can be a performance bottleneck
      # on shell startup. Perhaps we can optimize it away or make it faster.
      elif ! nvm_ensure_version_installed "${VERSION}"; then
        return $?
      fi

@ljharb
Copy link
Member

ljharb commented Nov 15, 2016

That seems much more reasonable :-)

@PeterDaveHello PeterDaveHello force-pushed the nvm-install-NA-fix branch 2 times, most recently from 539bbd4 to a676929 Compare November 15, 2016 09:02
@ljharb ljharb changed the title [WIP] [Fix] Handle 'N/A' version instead of asking to install it, fix #1304 [Fix] nvm install: Handle 'N/A' version instead of asking to install it Nov 16, 2016
@ljharb ljharb force-pushed the nvm-install-NA-fix branch from a676929 to fc7befc Compare November 16, 2016 06:50
@ljharb ljharb added the installing node Issues with installing node/io.js versions. label Nov 16, 2016
@ljharb ljharb force-pushed the nvm-install-NA-fix branch from fc7befc to 8c03637 Compare November 16, 2016 07:26
@ljharb ljharb merged commit 8c03637 into nvm-sh:master Nov 16, 2016
@PeterDaveHello PeterDaveHello deleted the nvm-install-NA-fix branch November 17, 2016 03:43
@manoharreddyporeddy
Copy link

Below commands worked:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
installing node Issues with installing node/io.js versions.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants