Install bash programmble completions for bash/zsh #753
Conversation
| @@ -177,6 +177,23 @@ nvm_check_global_modules() { | |||
| fi | |||
| } | |||
|
|
|||
| nvm_bash_completion() { | |||
| if [ -f "$HOME/.bashrc" ] || [ -f "$HOME/.bash_profile" ] || [ -f "$HOME/.zshrc" ] ; then | |||
There was a problem hiding this comment.
I wonder if it would be more efficient to do everything in the for loop below, including downloading the bash_completion file on the first iteration?
There was a problem hiding this comment.
We don't need to check the file for multiple times, do we?
There was a problem hiding this comment.
That's true - in your current code, you're doing up to 6 checks - 3 files twice, plus 1 bash_completion. With what I'm proposing, you'd do up to 6 as well - 3 files, plus 3 bash_completion. So I suppose it'd work out to be the same. However, you could use a local variable and set it to true when the file downloads, and then you'd only ever do up to 4 checks.
There was a problem hiding this comment.
Okay, updated, though the checking here won't have any performance issue I think.
|
Updated. |
| @@ -177,6 +177,26 @@ nvm_check_global_modules() { | |||
| fi | |||
| } | |||
|
|
|||
| nvm_bash_completion() { | |||
| NVM_PROFILE=$(nvm_detect_profile) | |||
| case "$NVM_PROFILE" in \ | |||
There was a problem hiding this comment.
My mistake - the \ isn't needed here, but it is needed after each of the pipes below
|
@ljharb I guess this is a out-dated PR, and already been implemented, right? |
|
It definitely hasn't been implemented - please reopen it and rebase if you're still interested in landing it :-) |
|
Oh okay, there are many conflicts, need more time :) |
|
@ljharb I rewrite it, is it good for you? |
|
I just added zsh and bash detection now :) |
| @@ -298,6 +298,7 @@ nvm_do_install() { | |||
| INSTALL_DIR="$(nvm_install_dir)" | |||
|
|
|||
| SOURCE_STR="\nexport NVM_DIR=\"$INSTALL_DIR\"\n[ -s \"\$NVM_DIR/nvm.sh\" ] && \\. \"\$NVM_DIR/nvm.sh\" # This loads nvm\n" | |||
| SOURCE_STR2="[ -s \"\$NVM_DIR/bash_completion\" ] && [ -n \"\$BASH_VERSION\" ] && [ -n \"\$ZSH_VERSION\" ] && \\. \"\$NVM_DIR/bash_completion\" # This loads nvm bash_completion\n" | |||
There was a problem hiding this comment.
adding an extra line to everyone's profiles that runtime-checks their shell seems subpar - can we not determine this prior to insertion?
There was a problem hiding this comment.
Could be, is detection with nvm_detect_profile and .bash/.zsh prefix good here?
There was a problem hiding this comment.
I just updated it.
There was a problem hiding this comment.
I think with your changes below, we no longer need the runtime shell checks here.
There was a problem hiding this comment.
Do you mean nvm_detect_profile?
There was a problem hiding this comment.
If bash or zsh, then we have string2, is that correct? 😄 Thanks,
There was a problem hiding this comment.
Basically yes, i think we're on the same page
There was a problem hiding this comment.
If I assign STR2 within the switch/case, the part we can not recognize profile wouldn't be able to print that string, what about to clear the string in switch/case for non-bash/zsh shell?
There was a problem hiding this comment.
you can assign to STR2 unconditionally. The parts I want to only happen conditionally are a) add the line to the profile file, and b) print out the line on stdout/stderr.
| @@ -302,13 +303,26 @@ nvm_do_install() { | |||
| echo " OR" | |||
| echo "=> Append the following lines to the correct file yourself:" | |||
| command printf "${SOURCE_STR}" | |||
| echo "=> Please also append the following lines to the if you are using bash/zsh shell:" | |||
There was a problem hiding this comment.
i'd like this line to only be output when the user is using them - in other words, in the same circumstances where "Appending bash_completion source string" happens.
There was a problem hiding this comment.
@ljharb I tried again and hope this would be correct now.
There was a problem hiding this comment.
Just realize that I can't do it since this message is for the users we can't detect their shell profile, if so, how could we know they are using bash or zsh I'll just drop this part now.
| @@ -323,6 +337,9 @@ nvm_do_install() { | |||
|
|
|||
| echo "=> Close and reopen your terminal to start using nvm or run the following to use it now:" | |||
| command printf "$SOURCE_STR" | |||
| if [ -n \"\$BASH_VERSION\" ] && [ -n \"\$ZSH_VERSION\" ]; then | |||
There was a problem hiding this comment.
i think this condition should match that on line 316
|
Updated! |
|
@PeterDaveHello would you mind checking the box for "allow maintainers to edit"? |
|
@ljharb done! |
No description provided.