feat: add shellcheck ci check#1179
Conversation
There was a problem hiding this comment.
range may contain multiple options, let's add a shellcheck disable.
There was a problem hiding this comment.
I'm not an expert here but converted range to array so that multiple values would be quoted correctly in (theoretical) case they'd contain IFS or glob characters. LMKWYT? Could also just shellcheck disable=SC2086. Thanks for quick feedback on these few items 😎
|
Let's merge main branch to make CI pass. |
There was a problem hiding this comment.
Should be "${range[*]}"?
There was a problem hiding this comment.
This is what SC2086 doc indicated and looks correct based on the bash docs.
If the subscript is ‘@’ or ‘’, the word expands to all members of the array name. These subscripts differ only when the word appears within double quotes. If the word is double-quoted, ${name[]} expands to a single word with the value of each array member separated by the first character of the IFS variable, and ${name[@]} expands each element of name to a separate word.
And indeed that seems to be the case:
$ ranges=( "range 1" "range 2" "range 3" ); for range in "${ranges[*]}"; do echo "$range"; done
range 1 range 2 range 3
$ ranges=( "range 1" "range 2" "range 3" ); for range in "${ranges[@]}"; do echo "$range"; done
range 1
range 2
range 3
|
@oikarinen |
hyperupcall
left a comment
hyperupcall
left a comment
There was a problem hiding this comment.
I spotted a few issues with the new changes. Could you go back and test any of the non-trivial changes to make sure they work?
There was a problem hiding this comment.
These new changes will cause git blame to fail whenever -w (or --ignore-whitespace) is not supplied at the command line.
There was a problem hiding this comment.
These new changes will now cause git add to fail in all cases (except the case where a single file is in list). Before, it would only fail on files that contained whitespace.
There was a problem hiding this comment.
Same comment as below.
Thanks, if the test suite doesn't cover the changes probably best approach is to just ignore and perhaps have an issue(s) in backlog to fix those to limit the size of the PR. |
Sounds good to me! |
There was a problem hiding this comment.
Bad indent and bad location?
There was a problem hiding this comment.
Hey, tabs vs spaces thing. Sorry I rebased the branch to main instead of merge from old habits :(
This was mentioned in some older PR.
oikarinen
force-pushed
the
shellcheck
branch
from
ba0bb78 to
329b9f8
Compare
oikarinen
force-pushed
the
shellcheck
branch
from
329b9f8 to
b5e760e
Compare
|
Great, thanks! |
3 participants
This was mentioned in some older PR. Autofixes in the same commit, some which seem actual bugs.