[install script] Refactors NVM_DIR #566
Conversation
xcambar
changed the title
| @@ -27,40 +27,44 @@ nvm_download() { | |||
| } | |||
|
|
|||
| install_nvm_from_git() { | |||
| local install_dir | |||
There was a problem hiding this comment.
let's name this NVM_INSTALL_DIR - in ksh the local doesn't work, so the var will remain afterwards.
There was a problem hiding this comment.
Isn't it better then that we unset it at the end of the function?
I've struggled to decide whether (not-environment) variables should be uppercased (as per the rest of the script) or lowercased (as per the accepted scripting conventions).
ksh is an edge case here. It should not be neglected, but maybe nvm could stick to the shell scripting conventions by usig lowercase for local variables and yet explicitely unset them for ksh.
It's mostly aesthetics, but it could help regular script writers to "feel home" quickly, and will leave ksh users with a cleaner environment, which could avoid bad side-effects at the same time.
There was a problem hiding this comment.
That's an idea as well, but not only is that a lot of extra cleanup, but that doesn't work if there's an existing variable set. If someone has install_dir set as an env var, even lowercase, we shouldn't overwrite it - and if we do, as in ksh, we may want to leave a value behind so that it can be used to trace where it's happening, instead of just clearing it.
ljharb
added
the
needs followup
|
@xcambar Are you still interested in landing this PR? |
|
@ljharb I'm still interested. I'll update the PR during the holidays. |
xcambar
force-pushed
the
nvm_install_dir
branch
2 times, most recently
from
5266383
to
620b8a7
Compare
|
Better late than never, the PR has been updated according to your comments and recent updates. Yours to review. |
| printf "\r=> " | ||
| cd "$NVM_DIR" && (command git fetch 2> /dev/null || { | ||
| echo >&2 "Failed to update nvm, run 'git fetch' in $NVM_DIR yourself." && exit 1 | ||
| cd "$install_dir" && (git fetch 2> /dev/null || { |
There was a problem hiding this comment.
I recently added command to ensure that git isn't aliased - can you ensure that all git commands are prefixed with command?
|
@xcambar do you have any interest in completing this PR? |
|
I can give it a try and rebase. |
|
Rebased and well :) |
| if [ -d "$NVM_DIR/.git" ]; then | ||
| echo "=> nvm is already installed in $NVM_DIR, trying to update using git" | ||
| local INSTALL_DIR | ||
| INSTALL_DIR=$(nvm_install_dir) |
There was a problem hiding this comment.
this needs to be surrounded in quotes, in case it has spaces in it
|
LGTM pending one comment :-) |
|
Done. I also cleaned up the function in Although I'm sure it's on purpose (ie, harmless), I'd like to bring to your attention that there are some non-quoted variables left in the script. If you want to see it fixed, I can open another PR for this, it won' take long. |
ljharb
added
installing node
|
Thanks, this looks great! Let's get the open install script PRs merged first, then we can fix the vars in another one. Let's rebase this down to one commit and get it in :-) |
ljharb
removed
the
needs rebase
After the PR:
NVM_DIR)NVM_DIRvariable, but uses a default value insteadNVM_DIRis used at a single point in the install script