Adding a symlink in .nvm to the current version. #467

Merged
merged 1 commit into from Jul 13, 2014

Conversation

Projects
None yet
4 participants
Contributor

jsdevel commented Jul 12, 2014

While working on some c++ add-ons, I thought it would be handy to have this change in my IDE.

Contributor

koenpunt commented Jul 12, 2014

Duplicate of #447

Collaborator

ljharb commented Jul 12, 2014

Thanks for the contribution! However, I think #447's approach is more appropriate - this will show up in the output of nvm ls, and current makes more sense to me (and is more consistent with rvm/rbenv) than node.

In addition, this would need tests before I'd merge it.

ljharb self-assigned this Jul 12, 2014

Contributor

jsdevel commented Jul 12, 2014

I added a test. @danielb2 I apologize for duplicating the effort. Feel free to use my tests if you'd rather have your Pull Request accepted.

@ljharb @koenpunt feedback is welcomed on the testing approach.

Collaborator

ljharb commented Jul 12, 2014

Thanks for the test! I think this could actually be a fast test - there's no need to actually do the install, just mkdir should be enough to verify. The bonus is that then it'd get tested on all the shells.

Contributor

jsdevel commented Jul 12, 2014

Done @ljharb

@ljharb ljharb commented on an outdated diff Jul 12, 2014

...use x" should create and change the "current" symlink
@@ -0,0 +1,24 @@
+#!/bin/bash
+
+. ../../nvm.sh
+
+nvm install 0.10
@ljharb

ljharb Jul 12, 2014

Collaborator

fast tests can't call nvm install - this should make a directory. Check out the other fast tests for examples. (same below)

Contributor

danielb2 commented Jul 12, 2014

My test is using the fast test already and should be good to go. Thoughts on it?

Contributor

jsdevel commented Jul 12, 2014

@ljharb updated the test to not install.

Contributor

jsdevel commented Jul 12, 2014

@danielb2 I found the test in #447 harder to understand.

Contributor

jsdevel commented Jul 12, 2014

@ljharb I removed the line in nvm.sh and the tests failed, so once the Travis build passes I think we're good to go (barring any other style / code changes recommended). @danielb2 again sorry for the dupe.

@ljharb ljharb commented on an outdated diff Jul 12, 2014

@@ -542,6 +542,7 @@ nvm() {
export NODE_PATH
export NVM_PATH="$NVM_DIR/$VERSION/lib/node"
export NVM_BIN="$NVM_DIR/$VERSION/bin"
+ rm -f $NVM_DIR/current && ln -s $NVM_DIR/$VERSION $NVM_DIR/current
@ljharb

ljharb Jul 12, 2014

Collaborator

not a blocker, but it would be good to wrap each of the file paths in double quotes, in case there's any spaces or special chars in $NVM_DIR

Collaborator

ljharb commented Jul 12, 2014

It looks like your test is checking that the link is created, and changed - but not checking what it points to. Could you add checks that its path includes the expected version number in both cases?

Contributor

jsdevel commented Jul 12, 2014

@ljharb done!

@jsdevel jsdevel commented on an outdated diff Jul 12, 2014

...use x" should create and change the "current" symlink
+ echo "Expected 'current' to point to $oldLink"
+ exit 1
+fi
+
+rm -rf ../../v0.11.13
+mkdir ../../v0.11.13
+nvm use 0.11.13
+
+newLink="$(readlink ../../current)"
+
+if [ "$(basename $newLink)" != 'v0.11.13' ];then
+ echo "Expected 'current' to point to $newLink"
+ exit 1
+fi
+
+if [ "$oldLink" == "$newLink" ];then
@jsdevel

jsdevel Jul 12, 2014

Contributor

@ljharb this test is probably redundant now. Thoughts?

Collaborator

ljharb commented Jul 12, 2014

This looks great. I'll think on it a bit before merging, but I'm thinking I will soon :-)

Contributor

jsdevel commented Jul 12, 2014

Thanks @ljharb! Please let me know if you'd like me to make any more updates. I look forward to having the symlink there in master for my c++ add-on development :)

@ljharb ljharb added a commit that referenced this pull request Jul 13, 2014

@ljharb ljharb Merge pull request #467 from jsdevel/adding-current-symlink
Adding a symlink in .nvm to the current version.

Fixes #430. Closes #447. Relates to #358. Fixes #355. Closes #313. Fixes #381.
8f66273

@ljharb ljharb merged commit 8f66273 into creationix:master Jul 13, 2014

1 check passed

continuous-integration/travis-ci The Travis CI build passed
Details

jsdevel deleted the jsdevel:adding-current-symlink branch Jul 14, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment