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

Include npm version when displaying results of switch. #722

Merged
merged 3 commits into from
Apr 12, 2015

Conversation

ajcrites
Copy link
Contributor

@ajcrites ajcrites commented Apr 9, 2015

When running nvm use, successful changing of versions lists the new node version. The npm version may also be switched, but this is not listed.

This commit updates nvm to display the npm version that was switched to alongside the node version

When running nvm use, successful changing of versions lists the new node version.  The npm version may also be switched, but this is not listed.

This commit updates nvm to display the npm version that was switched to alongside the node version
@ljharb
Copy link
Member

ljharb commented Apr 9, 2015

Not a bad idea at all! This will definitely fail tests, so please do update the relevant test files with the appropriate message.

else
echo "Now using node $VERSION"
echo -n "Now using node $VERSION"
Copy link
Member

Choose a reason for hiding this comment

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

echo -n isn't supported in sh (although it is supported in all other supported shells). Instead of using that, could we capture the npm version in a variable before the if/else, and interpolate that into a single echo command?

@ljharb ljharb added feature requests I want a new feature in nvm! needs followup We need some info or action from whoever filed this issue/PR. labels Apr 9, 2015
@ljharb
Copy link
Member

ljharb commented Apr 9, 2015

Also, please do note that versions of node older than v0.2.3 don't support npm at all, so we'd need to make sure that the npm text was only displayed if nvm_has "npm" after switching.

ajcrites added 2 commits April 9, 2015 02:12
* Update test string that checks printed node version to pass with additional npm version display
* Remove echo -n for compatibility with shells that do not support it
@ajcrites
Copy link
Contributor Author

ajcrites commented Apr 9, 2015

I missed this in the tests and I didn't actually think to run the tests. Silly me.

Fast tests pass for me. Slow tests do not pass on my system -- not even in the master branch (only the nvm_do_install test seems to fail).

I tried in both zsh and bash, disabling .npmrc and had not run nvm beforehand.

I was also not able to install 0.2.2 to test lack of npm specifically.

@ljharb
Copy link
Member

ljharb commented Apr 9, 2015

Not all the tests run reliably locally, so let's see how travis-ci does with them.

@ljharb
Copy link
Member

ljharb commented Apr 12, 2015

Sorry for the delay - this looks great, thanks!

ljharb added a commit that referenced this pull request Apr 12, 2015
Include npm version when displaying results of switch.
@ljharb ljharb merged commit 6a114ed into nvm-sh:master Apr 12, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature requests I want a new feature in nvm! needs followup We need some info or action from whoever filed this issue/PR.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants