Fix a few ShellCheck SC2086 & SC2155 in Wrapper's Linux script #1892
Conversation
vorburger
changed the title
|
Oups, hang on, at least one of the change proposed (automagically) here actually appears to break the wrapper... do not merge! |
vorburger
force-pushed
the
shellcheck
branch
from
c505234 to
ffbaca2
Compare
Found it - that It would be good to have a Please do carefully review to make sure that this doesn't break anything else (but I think as-is now it's fine). PS: I'm not sure (didn't check) if CI caught this - is there integration test coverage for the wrapper? |
| @@ -219,8 +219,10 @@ fi | |||
|
|
|||
| ## run it using command substitution to have just the user process once jbang is done | |||
| export JBANG_RUNTIME_SHELL=bash | |||
| export JBANG_STDIN_NOTTY=$([ -t 0 ] && echo "false" || echo "true") | |||
| JBANG_STDIN_NOTTY=$([ -t 0 ] && echo "false" || echo "true") | |||
There was a problem hiding this comment.
why did this line get split (and the other not)?
There was a problem hiding this comment.
See https://www.shellcheck.net/wiki/SC2155 ... so it's because this line uses $( ... ) and the other two do not.
|
Last time I tried shell check it was overly cautious but happy to see if we can improve :) this does change quotes so even though we do have some decent level of intégration tests in itest i'm not confident it tests for these. It would be great to actually have some test showing the issue shell check is worried about over just fixing it do reduce lint warnings. |
FWIW I did actually test these changes myself... ;-) and it seems to still work.
That would be cool (but I wouldn't want to be the one contributing it). To me, this is more like fixing a warning from a compiler or build tool - you typically perhaps wouldn't write a test? |
|
YOLO :) |
See https://www.shellcheck.net/wiki/SC2086 & https://www.shellcheck.net/wiki/SC2155.