Add shellcheck to pre-commit and fix warnings #4954
Conversation
gforsyth
added
improvement
| DGL_CHANNEL="dglteam/label/th23_cu118" | ||
| else | ||
| CONDA_CUDA_VERSION="12.1" |
There was a problem hiding this comment.
I love when these shellcheck PRs find unused variables 😁
ci/test_wheel.sh
Outdated
| @@ -28,5 +28,5 @@ else | |||
| --import-mode=append \ | |||
| --benchmark-disable \ | |||
| -k "not test_property_graph_mg and not test_bulk_sampler_io" \ | |||
| ./python/${package_name}/${python_package_name}/tests | |||
| ./python/"${package_name}"/"${python_package_name}"/tests | |||
There was a problem hiding this comment.
| ./python/"${package_name}"/"${python_package_name}"/tests | |
| "./python/${package_name}/${python_package_name}/tests" |
Since there isn't anything else here like wildcards that need to be evaluated, I have a mild preference for just wrapping the entire thing in double quotes, instead of having 2 inner sets of double quotes. Think that's a little easier to read and to modify.
ci/utils/nbtest.sh
Outdated
| jupyter nbconvert --to script "${NBFILENAME}" --output "${NBTMPDIR}"/"${NBNAME}"-test | ||
| echo "${MAGIC_OVERRIDE_CODE}" > "${NBTMPDIR}"/tmpfile | ||
| cat "${NBTESTSCRIPT}" >> "${NBTMPDIR}"/tmpfile | ||
| mv "${NBTMPDIR}"/tmpfile "${NBTESTSCRIPT}" |
There was a problem hiding this comment.
| jupyter nbconvert --to script "${NBFILENAME}" --output "${NBTMPDIR}"/"${NBNAME}"-test | |
| echo "${MAGIC_OVERRIDE_CODE}" > "${NBTMPDIR}"/tmpfile | |
| cat "${NBTESTSCRIPT}" >> "${NBTMPDIR}"/tmpfile | |
| mv "${NBTMPDIR}"/tmpfile "${NBTESTSCRIPT}" | |
| jupyter nbconvert --to script "${NBFILENAME}" --output "${NBTMPDIR}/${NBNAME}-test" | |
| echo "${MAGIC_OVERRIDE_CODE}" > "${NBTMPDIR}/tmpfile" | |
| cat "${NBTESTSCRIPT}" >> "${NBTMPDIR}/tmpfile" | |
| mv "${NBTMPDIR}/tmpfile" "${NBTESTSCRIPT}" |
Similar to my comments above.
.pre-commit-config.yaml
Outdated
| hooks: | ||
| - id: shellcheck | ||
| args: ["--severity=warning"] | ||
| files: ^ci/ |
There was a problem hiding this comment.
Could you expand this a bit? I'd really like to see it cover stuff like https://github.com/rapidsai/cugraph/blob/branch-25.04/datasets/get_test_data.sh and https://github.com/rapidsai/cugraph/blob/branch-25.04/scripts/dask/run-dask-process.sh too
If you want to just remove this exclusion and have it cover all shell scripts in this round, I don't mind that making the PR larger.
That'd also be one less repo to have to modify in the second round of this that covers build.sh scripts.
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(andother shlangs).
This PR adds a
pre-commithook to runshellcheckon all of thesh-langfiles in the
ci/directory, and the changes requested byshellcheckto makethe existing files pass the check.
xref: rapidsai/build-planning#135