Add auto completion support to zsh #1780

JemiloII · 2018-04-09T21:38:16Z

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.

Fixes nvm-sh#1707.

Using `$(basename "/$SHELL")` to detect the shell would also choose bash on mac

JemiloII · 2018-04-09T22:14:37Z

So updating the tests commit is me, I don't know why it shows as if I'm another user?

ljharb · 2018-04-10T05:01:48Z

@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.

ljharb · 2018-04-10T05:02:22Z

install.sh


-  if [ "$SHELLTYPE" = "bash" ]; then
+  if [ -n "$BASH_VERSION" ]; then


let's use "${BASH_VERSION-}" just in case set -u is set.

ljharb · 2018-04-10T05:02:29Z

install.sh

    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


let's use "${ZSH_VERSION-}" just in case set -u is set.

ljharb · 2018-04-10T05:03:17Z

install.sh

@@ -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"


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 :-)

reverted line

ljharb · 2018-04-10T05:03:29Z

install.sh

@@ -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


also ${ZSH_VERSION-} here

Removed in favor of using bash_completion

ljharb · 2018-04-10T05:04:06Z

install.sh

@@ -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'


is there a reason that this line needs to go in install.sh, instead of checking ZSH_VERSION inside the bash_completion file?

Actually no, so I moved it to bash completion. This helps clean up a lot of unnecessary lines.

ljharb · 2018-04-10T05:04:14Z

install.sh

@@ -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}"


2-space indentation

Removed in favor of using bash_completion

ljharb · 2018-04-10T05:04:17Z

install.sh

      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}"


2-space indentation

Removed in favor of using bash_completion

pecigonzalo · 2018-04-10T12:24:36Z

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.

ljharb · 2018-04-11T04:51:27Z

install.sh

@@ -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)" || {


ljharb · 2018-04-11T04:51:51Z

install.sh

@@ -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 #:


The colon doesn’t do anything inside a comment, is there a reason this needs to be here?

yeah probably from exiting vim.

ljharb

Thanks!

JemiloII added 2 commits April 9, 2018 14:54

[Fix] Add autocompletion support to zsh

68fe26a

Fixes nvm-sh#1707.

[Fix] install.sh: Detect profile with $BASH_VERSION and $ZSH_VERSION

8b71861

Using `$(basename "/$SHELL")` to detect the shell would also choose bash on mac

ljharb requested changes Apr 10, 2018

View reviewed changes

ljharb requested changes Apr 11, 2018

View reviewed changes

ljharb added the shell: zsh label Apr 11, 2018

ljharb approved these changes Apr 11, 2018

View reviewed changes

ljharb force-pushed the patch-1 branch from 3788e9c to 8b71861 Compare April 11, 2018 17:05

ljharb merged commit 8b71861 into nvm-sh:master Apr 11, 2018

JemiloII mentioned this pull request Apr 12, 2018

Install from zsh doesn't install to bash. Install from bash doesn't install to bash #1717

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add auto completion support to zsh #1780

Add auto completion support to zsh #1780

JemiloII commented Apr 9, 2018 •

edited by ljharb

Loading

JemiloII commented Apr 9, 2018

ljharb commented Apr 10, 2018

ljharb Apr 10, 2018

JemiloII Apr 10, 2018

ljharb Apr 10, 2018

JemiloII Apr 10, 2018

ljharb Apr 10, 2018

JemiloII Apr 10, 2018

ljharb Apr 10, 2018

JemiloII Apr 10, 2018

ljharb Apr 10, 2018

JemiloII Apr 10, 2018

ljharb Apr 10, 2018

JemiloII Apr 10, 2018

ljharb Apr 10, 2018

JemiloII Apr 10, 2018

pecigonzalo commented Apr 10, 2018

ljharb Apr 11, 2018

JemiloII Apr 11, 2018

ljharb Apr 11, 2018

JemiloII Apr 11, 2018

ljharb left a comment


		if [ "$SHELLTYPE" = "bash" ]; then
		if [ -n "$BASH_VERSION" ]; then

Add auto completion support to zsh #1780

Add auto completion support to zsh #1780

Conversation

JemiloII commented Apr 9, 2018 • edited by ljharb Loading

JemiloII commented Apr 9, 2018

ljharb commented Apr 10, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pecigonzalo commented Apr 10, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ljharb left a comment

Choose a reason for hiding this comment

JemiloII commented Apr 9, 2018 •

edited by ljharb

Loading