shellcheck: Run on .nf scripts

[zeehio]

I found a bug on a shell script embedded in a script: section of a nextflow file from one of the nf-core modules. I provided a fix on this PR:

• https://github.com/nf-core/modules/pull/8177

@mashehu suggested me to provide a unit test. I decided it would be better to use shellcheck on the script, to automatically report all warnings and linting errors.

Why not do it on all shell scripts in the nf-core/modules? I guess it could run on the CI if you wanted.

The included tests/test_shellcheck.py is a python script that extracts all the bash scripts from .nf files and runs shellcheck on them. Even if the scripts extracts each script block into a .sh file and runs shellcheck on it, the script parses shellcheck output and maps it to the original file and line. Besides, it reports the linting information on both markdown and json formats.

Since the script: sections use variables defined earlier in the nf script, and shellcheck is not aware of those variables, I have disabled the "undefined variable" warning (SC2154), because it gave lots of false positives.

I ran python tests/test_shellcheck.py and this was the generated output:

shellcheck_output.json shellcheck_output.md

For your convenience, I uploaded the markdown file to a gist, so you can easily preview it:

https://gist.github.com/zeehio/305177fcbec25659211feb2276d5fe35

The report is long, and probably many of the warnings are harmless. Still, PR #8177 is proof that a linter would be beneficial to some of the shell scripts, and I believe shellcheck is quite widely used.

Disclaimer: I got support from an LLM (copilot) to speed up the development of this script.

PR checklist

• [x] This comment contains a description of changes (with reason).
• [x] If you've fixed a bug or added code that should be tested, add tests!
• [ ] If you've added a new tool - have you followed the module conventions in the contribution docs
• [ ] If necessary, include test data in your PR.
• [ ] Remove all TODO statements.
• [ ] Emit the versions.yml file.
• [ ] Follow the naming conventions.
• [ ] Follow the parameters requirements.
• [ ] Follow the input/output options guidelines.
• [ ] Add a resource label
• [ ] Use BioConda and BioContainers if possible to fulfil software requirements.
• Ensure that the test works with either Docker / Singularity. Conda CI tests can be quite flaky:
  • For modules:
    • [ ] nf-core modules test <MODULE> --profile docker
    • [ ] nf-core modules test <MODULE> --profile singularity
    • [ ] nf-core modules test <MODULE> --profile conda
  • For subworkflows:
    • [ ] nf-core subworkflows test <SUBWORKFLOW> --profile docker
    • [ ] nf-core subworkflows test <SUBWORKFLOW> --profile singularity
    • [ ] nf-core subworkflows test <SUBWORKFLOW> --profile conda

[SPPearce]

I think you need to disable SC2086 too, as most of the output you shared is things like:

${args} \
    ^-----^ SC2086 (info): Double quote to prevent globbing and word splitting.

[SPPearce]

Actually, the $variable is evaluating as a Nextflow variable, not within the shell context, so several of the checks aren't relevant.

[zeehio]

I think you need to disable SC2086 too, as most of the output you shared is things like:

${args} \
    ^-----^ SC2086 (info): Double quote to prevent globbing and word splitting.

There are lots of these warnings, but not addressing this warning is what caused #8177 in the first place, so I would not want to disable it.

[edmundmiller]

Cool idea! Could you try wiring it up to pre-commit like this? https://github.com/nf-core/modules/blob/8e6e124ee1e7b9d6b8dacca408a936d56865322f/.pre-commit-config.yaml#L4-L10

[zeehio]

I just rebased this PR, but I guess I need help setting up the pre-commit hook, or take some time to figure out how pre-commit works...

[SPPearce]

Paging @edmundmiller

[zeehio]

With the help of @mashehu I was able to provide a pre-commit hook that works well and installs the dependencies it requires