-
-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
Add auto completion support to zsh #1780
Conversation
Using `$(basename "/$SHELL")` to detect the shell would also choose bash on mac
So updating the tests commit is me, I don't know why it shows as if I'm another user? |
@JemiloII all the other commits have your "jemiloii.com" email address; the last one has your gmail. If you add your gmail to your github account, then it will connect the commit properly. |
install.sh
Outdated
|
||
if [ "$SHELLTYPE" = "bash" ]; then | ||
if [ -n "$BASH_VERSION" ]; 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.
let's use "${BASH_VERSION-}"
just in case set -u
is set.
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.
updated
install.sh
Outdated
if [ -f "$HOME/.bashrc" ]; then | ||
DETECTED_PROFILE="$HOME/.bashrc" | ||
elif [ -f "$HOME/.bash_profile" ]; then | ||
DETECTED_PROFILE="$HOME/.bash_profile" | ||
fi | ||
elif [ "$SHELLTYPE" = "zsh" ]; then | ||
elif [ -n "$ZSH_VERSION" ]; 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.
let's use "${ZSH_VERSION-}"
just in case set -u
is set.
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.
updated
install.sh
Outdated
@@ -329,7 +327,9 @@ nvm_do_install() { | |||
local PROFILE_INSTALL_DIR | |||
PROFILE_INSTALL_DIR="$(nvm_install_dir | command sed "s:^$HOME:\$HOME:")" | |||
|
|||
SOURCE_STR="\\nexport NVM_DIR=\"${PROFILE_INSTALL_DIR}\"\\n[ -s \"\$NVM_DIR/nvm.sh\" ] && \\. \"\$NVM_DIR/nvm.sh\" # This loads nvm\\n" | |||
SOURCE_STR="\\nexport NVM_DIR=\"${PROFILE_INSTALL_DIR}\"\\n[ -s \"\$NVM_DIR/nvm.sh\" ] && \\. \"\$NVM_DIR/nvm.sh\" # This loads nvm\\n\n" |
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.
note that the newlines here are double-escaped - \\n
instead of \n
- but also, let's revert this line entirely, and if ZSH_COMPLETION_STR
needs a leading newline, then it can prepend \\n
to itself :-)
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.
reverted line
install.sh
Outdated
@@ -357,13 +357,19 @@ nvm_do_install() { | |||
# shellcheck disable=SC2016 | |||
if ${BASH_OR_ZSH} && ! command grep -qc '$NVM_DIR/bash_completion' "$NVM_PROFILE"; then | |||
echo "=> Appending bash_completion source string to $NVM_PROFILE" | |||
if [ -n "$ZSH_VERSION" ]; 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.
also ${ZSH_VERSION-}
here
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.
Removed in favor of using bash_completion
install.sh
Outdated
@@ -329,7 +327,9 @@ nvm_do_install() { | |||
local PROFILE_INSTALL_DIR | |||
PROFILE_INSTALL_DIR="$(nvm_install_dir | command sed "s:^$HOME:\$HOME:")" | |||
|
|||
SOURCE_STR="\\nexport NVM_DIR=\"${PROFILE_INSTALL_DIR}\"\\n[ -s \"\$NVM_DIR/nvm.sh\" ] && \\. \"\$NVM_DIR/nvm.sh\" # This loads nvm\\n" | |||
SOURCE_STR="\\nexport NVM_DIR=\"${PROFILE_INSTALL_DIR}\"\\n[ -s \"\$NVM_DIR/nvm.sh\" ] && \\. \"\$NVM_DIR/nvm.sh\" # This loads nvm\\n\n" | |||
ZSH_COMPLETION_STR='# Load compinit/compdef\n autoload -U compinit\n compinit\n\n' |
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.
is there a reason that this line needs to go in install.sh
, instead of checking ZSH_VERSION
inside the bash_completion
file?
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.
Actually no, so I moved it to bash completion. This helps clean up a lot of unnecessary lines.
install.sh
Outdated
@@ -380,6 +386,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 ${BASH_OR_ZSH} ; then | |||
if [ -n "$ZSH_VERSION" ]; then | |||
command printf "${ZSH_COMPLETION_STR}" |
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.
2-space indentation
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.
Removed in favor of using bash_completion
install.sh
Outdated
command printf "$COMPLETION_STR" >> "$NVM_PROFILE" | ||
else | ||
echo "=> bash_completion source string already in ${NVM_PROFILE}" | ||
fi | ||
fi | ||
if ${BASH_OR_ZSH} && [ -z "${NVM_PROFILE-}" ] ; then | ||
echo "=> Please also append the following lines to the if you are using bash/zsh shell:" | ||
if [ -n "$ZSH_VERSION" ]; then | ||
command printf "${ZSH_COMPLETION_STR}" |
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.
2-space indentation
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.
Removed in favor of using bash_completion
Nice work, I'll give it a look later today, I did not implement a solution yet as loading it might interfere with other things, as load order is important for this. |
install.sh
Outdated
@@ -101,7 +101,7 @@ install_nvm_from_git() { | |||
exit 2 | |||
} | |||
command git --git-dir="${INSTALL_DIR}/.git" remote add origin "$(nvm_source)" 2> /dev/null \ | |||
|| command git --git-dir="${INSTALL_DIR}/.git" remote set-url origin "$(nvm_source)" || { | |||
|| command git --git-dir="${INSTALL_DIR}/.git" remote set-nrl origin "$(nvm_source)" || { |
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.
Typo?
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.
yeah.
install.sh
Outdated
@@ -397,4 +396,4 @@ nvm_reset() { | |||
|
|||
[ "_$NVM_ENV" = "_testing" ] || nvm_do_install | |||
|
|||
} # this ensures the entire script is downloaded # | |||
} # this ensures the entire script is downloaded #: |
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.
The colon doesn’t do anything inside a comment, is there a reason this needs to be here?
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.
yeah probably from exiting vim.
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.
Thanks!
For more info visit -> #1707
Credit goes to @pecigonzalo for figuring out the issue, I just did the work to put it in the install script and made sure that it did not conflict with bash.
Just run the install script in zsh to see working. Also please test that the install script still works in bash :)
Fixes #1707.