ShellCheck v0.4.7 fixes #1684

PeterDaveHello · 2017-12-09T10:48:42Z

ShellCheck v0.4.7 just released 6 hours ago and hopefully will be deployed on Travis CI soon.
This is the fix for SC2207 and SC1117

ljharb · 2017-12-09T17:55:20Z

Until I can use the updated shellcheck locally, I'm not going to be comfortable landing this.

ljharb · 2017-12-09T21:56:31Z

nvm.sh

@@ -53,7 +53,7 @@ nvm_command_info() {
    INFO="$(which "${COMMAND}") ($(type "${COMMAND}" | command awk '{ $1=$2=$3=$4="" ;print }' | command sed -e 's/^\ *//g' -Ee "s/\`|'//g" ))"
  elif type "${COMMAND}" | nvm_grep -q "^${COMMAND} is an alias for"; then
    INFO="$(which "${COMMAND}") ($(type "${COMMAND}" | command awk '{ $1=$2=$3=$4=$5="" ;print }' | command sed 's/^\ *//g'))"
-  elif type "${COMMAND}" | nvm_grep -q "^${COMMAND} is \/"; then
+  elif type "${COMMAND}" | nvm_grep -q "^${COMMAND} is \\/"; then


is the intention here to grep for $COMMAND is \/ or $COMMAND is /?

https://github.com/koalaman/shellcheck/wiki/SC1117 implies that it probably should have been $COMMAND is /?

$COMMAND is /, the effect here is the same, it won't be $COMMAND is \/

Doesn't \\ put a literal backslash into the text?

I thought so but I don't think so now. From SC1117 wiki, looks like it's because we use " not '.

ok, so '\n' is correct, and "\\n" is the double-quoted equivalent?

ljharb · 2017-12-09T21:57:22Z

nvm.sh

@@ -645,16 +645,16 @@ nvm_print_formatted_alias() {
  DEST_FORMAT='%s'
  VERSION_FORMAT='%s'
  local NEWLINE
-  NEWLINE="\n"
+  NEWLINE="\\n"


This just doesn't seem right to me. The intention is to insert a literal newline; not to literally add a backslash and an n

Yes, it's newline, that's the tricky point I think.

\\n isn't a newline tho :-/

Really? I tested it with this code block and it looks fine:

export NEWLINE="\n" command printf -- "${NEWLINE}" unset NEWLINE export NEWLINE="\\n" command printf -- "${NEWLINE}" unset NEWLINE command printf -- "${NEWLINE}"

I'm confused; does that mean \n and \\n are the same? Or is the former a literal newline, and the latter a literal \n that the shell later turns into a newline?

Yeah, I think they are the same as the wiki mentions, the old usage may be wrong, and just fallback as what we want, that's the reason why they output the same.

so on this line; can it be replaced with '\n'?

ljharb · 2017-12-09T21:58:12Z

bash_completion

@@ -10,7 +10,7 @@ __nvm_generate_completion()
 {
  declare current_word
  current_word="${COMP_WORDS[COMP_CWORD]}"
-  COMPREPLY=($(compgen -W "$1" -- "$current_word"))
+  COMPREPLY=("$(compgen -W "$1" -- "$current_word")")


is not the intention to split the output here?

ljharb · 2017-12-10T18:51:28Z

nvm.sh

          " \
-          -e "s#^\([^/]\{1,\}\)/\(.*\)\$#\2.\1#;" \
+          -e "s#^\\([^/]\\{1,\\}\\)/\\(.*\\)\$#\\2.\\1#;" \


instead of making this line less readable, could it be single-quoted instead?

I tested it and it seems wrong lol.

ljharb · 2017-12-10T18:51:37Z

nvm.sh

        | command sort -t. -u -k 1.2,1n -k 2,2n -k 3,3n \
        | command sed "
-            s#\(.*\)\.\([^\.]\{1,\}\)\$#\2-\1#;
+            s#\\(.*\\)\\.\\([^\\.]\\{1,\\}\\)\$#\\2-\\1#;


This one combined with shell variable below, I'm not very sure how to deal with it lol

ljharb · 2017-12-13T18:43:12Z

nvm.sh

  if [ "_${DEFAULT}" = '_true' ]; then
-    NEWLINE=" (default)\n"
+    NEWLINE=" (default)\\n"


also can this be single-quoted, and then the double backslash removed?

ljharb · 2017-12-13T18:43:17Z

nvm.sh

  fi
  local ARROW
  ARROW='->'
  if [ -z "${NVM_NO_COLORS}" ] && nvm_has_colors; then
    ARROW='\033[0;90m->\033[0m'
    if [ "_${DEFAULT}" = '_true' ]; then
-      NEWLINE=" \033[0;37m(default)\033[0m\n"
+      NEWLINE=" \\033[0;37m(default)\\033[0m\\n"


also can this be single-quoted, and then the double backslash removed?

ljharb · 2017-12-13T18:43:26Z

nvm.sh

@@ -1021,25 +1021,23 @@ nvm_ls() {
      PATTERN='v'
      SEARCH_PATTERN='.*'
    else
-      SEARCH_PATTERN="$(echo "${PATTERN}" | command sed "s#\.#\\\.#g;")"
+      SEARCH_PATTERN="$(echo "${PATTERN}" | command sed "s#\\.#\\\\.#g;")"


PeterDaveHello · 2017-12-13T19:05:27Z

Hope this one can be good, replaced those patterns without a variable to use the single quote now.

ljharb

Thanks!

PeterDaveHello · 2017-12-13T19:27:33Z

Noticed that SC2016 should be disabled for $COMPLETION_STR in install.sh , updated line 329.

PeterDaveHello · 2017-12-13T20:40:58Z

Tests passed!

…fixes ShellCheck v0.4.7 fixes

ljharb self-assigned this Dec 9, 2017

ljharb reviewed Dec 9, 2017

View reviewed changes

PeterDaveHello force-pushed the ShellCheck-v0.4.7-fixes branch from 4fd4e8e to 6632245 Compare December 10, 2017 07:21

ljharb reviewed Dec 10, 2017

View reviewed changes

PeterDaveHello force-pushed the ShellCheck-v0.4.7-fixes branch 2 times, most recently from de43f24 to 6632245 Compare December 13, 2017 17:28

[Fix] Disable ShellCheck SC2207 in bash_completion line 14

fce7f4e

PeterDaveHello force-pushed the ShellCheck-v0.4.7-fixes branch from 6632245 to d1fe2a9 Compare December 13, 2017 18:19

ljharb reviewed Dec 13, 2017

View reviewed changes

PeterDaveHello force-pushed the ShellCheck-v0.4.7-fixes branch from d1fe2a9 to 2d2f643 Compare December 13, 2017 19:04

ljharb approved these changes Dec 13, 2017

View reviewed changes

[Fix] ShellCheck SC1117 in nvm.sh & install.sh

2a8a81d

PeterDaveHello force-pushed the ShellCheck-v0.4.7-fixes branch from 2d2f643 to 2a8a81d Compare December 13, 2017 19:26

ljharb merged commit 2a25943 into nvm-sh:master Dec 13, 2017

PeterDaveHello deleted the ShellCheck-v0.4.7-fixes branch December 14, 2017 03:52

edwmurph pushed a commit to edwmurph/nvm that referenced this pull request Apr 9, 2018

Merge pull request nvm-sh#1684 from PeterDaveHello/ShellCheck-v0.4.7-…

cac755f

…fixes ShellCheck v0.4.7 fixes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ShellCheck v0.4.7 fixes #1684

ShellCheck v0.4.7 fixes #1684

PeterDaveHello commented Dec 9, 2017

ljharb commented Dec 9, 2017

ljharb Dec 9, 2017

PeterDaveHello Dec 10, 2017

ljharb Dec 10, 2017

PeterDaveHello Dec 10, 2017

ljharb Dec 10, 2017

PeterDaveHello Dec 13, 2017

ljharb Dec 9, 2017

PeterDaveHello Dec 10, 2017

ljharb Dec 10, 2017

PeterDaveHello Dec 10, 2017

ljharb Dec 10, 2017

PeterDaveHello Dec 10, 2017

ljharb Dec 13, 2017

ljharb Dec 9, 2017

PeterDaveHello Dec 10, 2017

ljharb Dec 10, 2017

PeterDaveHello Dec 13, 2017

ljharb Dec 10, 2017

PeterDaveHello Dec 13, 2017

ljharb Dec 13, 2017

ljharb Dec 13, 2017

ljharb Dec 13, 2017

PeterDaveHello commented Dec 13, 2017

ljharb left a comment

PeterDaveHello commented Dec 13, 2017 •

edited by ljharb

PeterDaveHello commented Dec 13, 2017

ShellCheck v0.4.7 fixes #1684

ShellCheck v0.4.7 fixes #1684

Conversation

PeterDaveHello commented Dec 9, 2017

ljharb commented Dec 9, 2017

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

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

PeterDaveHello commented Dec 13, 2017

ljharb left a comment

Choose a reason for hiding this comment

PeterDaveHello commented Dec 13, 2017 • edited by ljharb

PeterDaveHello commented Dec 13, 2017

PeterDaveHello commented Dec 13, 2017 •

edited by ljharb