Skip to content
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

Improve shellcheck coverage #1596

Merged
merged 3 commits into from
Jun 13, 2024
Merged

Improve shellcheck coverage #1596

merged 3 commits into from
Jun 13, 2024

Conversation

edmorley
Copy link
Member

@edmorley edmorley commented Jun 13, 2024

  • Switches to using git ls-files instead of a hardcoded list of scripts to lint (shellcheck doesn't support recursively scanning for scripts itself). This means a few files that were not previously linted now are, so need fixes.
  • Adds a .shellcheckrc which is used to enable optional shellcheck checks via enable=all. Most of these checks either already passed or have been fixed in this PR - however, a few others need a closer look so have been disabled for now to reduce the size of this PR.
  • As part of doing this I discovered an NLTK bug with setting NLTK_DATA after Misc bash script refactoring #1595, which has now been fixed. Since that change was not yet released, I've not mentioned the fix here in the changelog.

Shellcheck's optional checks as of v0.10.0:

$ shellcheck --list-optional
name:    add-default-case
desc:    Suggest adding a default case in `case` statements
example: case $? in 0) echo 'Success';; esac
fix:     case $? in 0) echo 'Success';; *) echo 'Fail' ;; esac

name:    avoid-nullary-conditions
desc:    Suggest explicitly using -n in `[ $var ]`
example: [ "$var" ]
fix:     [ -n "$var" ]

name:    check-extra-masked-returns
desc:    Check for additional cases where exit codes are masked
example: rm -r "$(get_chroot_dir)/home"
fix:     set -e; dir="$(get_chroot_dir)"; rm -r "$dir/home"

name:    check-set-e-suppressed
desc:    Notify when set -e is suppressed during function invocation
example: set -e; func() { cp *.txt ~/backup; rm *.txt; }; func && echo ok
fix:     set -e; func() { cp *.txt ~/backup; rm *.txt; }; func; echo ok

name:    check-unassigned-uppercase
desc:    Warn when uppercase variables are unassigned
example: echo $VAR
fix:     VAR=hello; echo $VAR

name:    deprecate-which
desc:    Suggest 'command -v' instead of 'which'
example: which javac
fix:     command -v javac

name:    quote-safe-variables
desc:    Suggest quoting variables without metacharacters
example: var=hello; echo $var
fix:     var=hello; echo "$var"

name:    require-double-brackets
desc:    Require [[ and warn about [ in Bash/Ksh
example: [ -e /etc/issue ]
fix:     [[ -e /etc/issue ]]

name:    require-variable-braces
desc:    Suggest putting braces around all variable references
example: var=hello; echo $var
fix:     var=hello; echo ${var}

See also:
https://github.com/koalaman/shellcheck/blob/master/shellcheck.1.md

@edmorley edmorley self-assigned this Jun 13, 2024
@edmorley edmorley force-pushed the more-shellcheck branch 2 times, most recently from 683de7f to 47520fa Compare June 13, 2024 10:10
@edmorley edmorley marked this pull request as ready for review June 13, 2024 10:15
@edmorley edmorley requested a review from a team as a code owner June 13, 2024 10:15
@edmorley edmorley force-pushed the more-shellcheck branch 2 times, most recently from 693dba4 to 4427fb0 Compare June 13, 2024 10:44
* Switches to using `git ls-files` instead of a hardcoded list of scripts to
  lint (shellcheck doesn't support recursively scanning for scripts itself).
  This means a few files that were not previously linted now are, so need
  fixes.
* Adds a `.shellcheckrc` which is used to enable optional shellcheck
  checks via `enable=all`. Most of these checks either already passed
  or have been fixed in this PR - however, a few others need a closer
  look so have been disabled for now to reduce the size of this PR.
* As part of doing this I discovered an NLTK bug with setting `NLTK_DATA`
  after #1595, which has now been fixed. Since that change was not yet
  released, I've not mentioned the fix here in the changelog.
@edmorley edmorley enabled auto-merge (squash) June 13, 2024 11:03
Makefile Outdated Show resolved Hide resolved
bin/steps/collectstatic Show resolved Hide resolved
bin/steps/python Outdated Show resolved Hide resolved
bin/utils Outdated Show resolved Hide resolved
edmorley and others added 2 commits June 13, 2024 15:57
Co-authored-by: David Zülke <dzuelke@salesforce.com>
Co-authored-by: David Zülke <dzuelke@salesforce.com>
@edmorley edmorley merged commit beb3328 into main Jun 13, 2024
6 checks passed
@edmorley edmorley deleted the more-shellcheck branch June 13, 2024 15:02
@heroku-linguist heroku-linguist bot mentioned this pull request Jun 17, 2024
edmorley added a commit that referenced this pull request Oct 3, 2024
In #1596 a number of optional Shellcheck rules were enabled.
However, there were some which were deferred due to the
number of fixes required, and so were left disabled via the
`.shellcheckrc` file.

In order that we can get the benefit of these rules for new code,
I've removed the global disabling in favour of per file or per line
`disable` directives (and in some cases, fixing outright). These
can then be fixed piecemeal as refactorings occur later.

Of note, one of these optional Shellcheck rules (SC2311) would
have saved me a fair amount of debugging time earlier today in
the new Python version handling implementation.

GUS-W-16898648.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants