-
-
Notifications
You must be signed in to change notification settings - Fork 8.1k
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
Implements nvm which #583
Implements nvm which #583
Conversation
"which" ) | ||
INPUT=$2 | ||
|
||
if [ "_$2" != '_system' ]; then |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nvm_version system
is system
- might be simpler to just always send $INPUT
through nvm_version
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- assuming that
$INPUT
is defined, of course
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did it this way to allow for nvm which
with no arguments. The idea is that it would show the default.
Do you think it's a bad idea? It was more work than it was worth :/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Conventional behavior for all the nvm
commands is that when a version isn't specified, it follows these steps in order:
- call
nvm_rc_version
and if[ -n "$NVM_RC_VERSION" ]
, then use that. - use
nvm alias default
if set - error out and show help
This is great! It would also be ideal if |
@ljharb did you have any more concerns? |
fi | ||
|
||
if [ "_$VERSION" = '_system' ]; then | ||
if nvm_has_system_node && nvm deactivate >/dev/null 2>&1; then |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this nvm deactivate
means that nvm which system
will end up deactivating the current shell. I think you want to remove deactivate
here, and change the below line to echo $(nvm use system && echo dirname $(which node))
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the purpose of the nvm use system
in your suggestion? I wouldn't want to change anything since the purpose of nvm which
is just to display information
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I have a version activated, the subshell will also have that version activated - nvm use system
in the subshell will deactivate nvm in the subshell without affecting the parent/current shell.
Sorry for the delay; I was out of town. In addition to the comments above, could you add test coverage for Also, this will need to be freshly rebased on top of master. Thanks! |
|
||
if [ "_$VERSION" = '_system' ]; then | ||
if nvm_has_system_node >/dev/null 2>&1; then | ||
echo $(dirname `which node`) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Per the previous comments - with a system node of 0.10.33, nvm use 0.9 && nvm which system
would return 0.9.x
, unless as the subshell command you have "$(nvm use system && dirname "$(which node)")"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, thanks
FYI, the tests files with spaces are a real pain. Have you considered using underscores instead? |
The file names are used in the test results, so underscores would make those really unreadable. |
Should be easy to substitute the underscores for spaces when printing the output. Either way, it's a trade-off I'd do. It's been really annoying to deal with the files with spaces for me. |
|
||
if [ "_$VERSION" = '_system' ]; then | ||
if nvm_has_system_node >/dev/null 2>&1; then | ||
echo $(nvm use system && echo dirname $(which node)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This line should be echo "$(nvm use system 2>&1 >/dev/null && dirname "$(which node)")"
.
Note the removed "echo", the suppression of nvm use system
output, and the double quotes around paths that could contain spaces.
Other than this one comment, and adding additional tests, this works perfectly, thanks! Your persistence is appreciated. |
Hm, I still think that just typing Right now it uses the rest of the default behavior and checks for |
My position is that since it starts with |
Let's compare Typing The way you're asking me to do What users expect is important too. And I expect Remember the purpose of this PR. It's so that it can be used with other shells and be free standing. So if a user does |
When a In the case of no |
The goal is to set the PATH. How would |
Right - when the PATH is not set, How could |
Let's start over because I feel we're not getting anywhere now. The current limitation of The idea is to have a wrapper for shells. Let's take I imagined calling What is your thinking? |
Thanks, you're right we were going in circles :-) OK, so you're saying that in a shell where In that context, why do you need an (Note I'm not saying that |
Not quite. There's no active |
I meant in the wrapper shell ( |
Wow it got hairy :) At this point I would say that |
Right - but At any rate,
The first seems consistent but valueless. The second is consistent with The third doesn't seem to add much value, since anywhere it would work, Thoughts? |
I just tested this, and it seems to confirm with option 2 above. I'm going to merge this - thanks for your persistence and contribution! |
No description provided.