Add shellcheck to pre-commit and fix warnings #1788
Conversation
gforsyth
added
non-breaking
| CURRENT_MAJOR=$(echo $CURRENT_TAG | awk '{split($0, a, "."); print a[1]}') | ||
| CURRENT_MINOR=$(echo $CURRENT_TAG | awk '{split($0, a, "."); print a[2]}') | ||
| CURRENT_PATCH=$(echo $CURRENT_TAG | awk '{split($0, a, "."); print a[3]}') | ||
| CURRENT_SHORT_TAG=${CURRENT_MAJOR}.${CURRENT_MINOR} |
There was a problem hiding this comment.
These variables are used in a few rapids projects (cudf, cuml, cuvs) but not others. Happy to keep them and just ignore the shellcheck warnings if we'd like to have these available across projects.
There was a problem hiding this comment.
There are a lot of discrepancies in the update-version.sh scripts across repos. We will want to keep an eye out for any best practices that we can unify across repos.
In the meantime, I am okay with removing unused variables.
| RAPIDS_VERSION_MAJOR_MINOR="$(rapids-version-major-minor)" | ||
| export RAPIDS_VERSION_MAJOR_MINOR |
There was a problem hiding this comment.
I do not like this style because it requires two lines where only one was used before. export VAR="value" seems like a very common and well-understood pattern in shell scripts. Is there a justification for why this is better practice?
There was a problem hiding this comment.
doing it as a single line masks the return value of the captured command. so if the captured command fails, even with set -e, the script won't fail because the export still succeeds.
There was a problem hiding this comment.
TIL! I need to read the shellcheck docs, probably.
There was a problem hiding this comment.
bash is a gnarly language. footguns all the way down
| CURRENT_MAJOR=$(echo $CURRENT_TAG | awk '{split($0, a, "."); print a[1]}') | ||
| CURRENT_MINOR=$(echo $CURRENT_TAG | awk '{split($0, a, "."); print a[2]}') | ||
| CURRENT_PATCH=$(echo $CURRENT_TAG | awk '{split($0, a, "."); print a[3]}') | ||
| CURRENT_SHORT_TAG=${CURRENT_MAJOR}.${CURRENT_MINOR} |
There was a problem hiding this comment.
There are a lot of discrepancies in the update-version.sh scripts across repos. We will want to keep an eye out for any best practices that we can unify across repos.
In the meantime, I am okay with removing unused variables.
Description
shellcheckis a fast, static analysis tool for shell scripts. It's good atflagging up unused variables, unintentional glob expansions, and other potential
execution and security headaches that arise from the wonders of
bash(and other shlangs).This PR adds a
pre-commithook to runshellcheckon all of thesh-langfiles in theci/directory, and the changes requested byshellcheckto make the existing files pass the check.xref: rapidsai/build-planning#135
Checklist