Skip to content

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