search

snok / install-poetry

Github action for installing and configuring Poetry
MIT License
573 stars 53 forks source link

Lint shell scripts #91

Closed sondrelg closed 1 year ago

sondrelg commented 1 year ago

Adds shfmt and shellcheck to our linter setup. Only the ${INSTALLATION_ARGUMENTS} call in main.sh:20 looks like it cannot be formatted according to shellcheck, so I've added ignore statements for those. This mainly re-formats the old script files.

Also sets set -eo pipefail to make the action error properly. The "command poetry does not exist" output for when the installation failed is not very user friendly.

miigotu commented 1 year ago

Was using tabs instead of spaces intentional?

I use shellcheck in my ci (and locally)

In the ci use format=diff and locally let it use the gcc format for output.

https://github.com/SickChill/sickchill/blob/master/.github/workflows/pythonpackage.yml#L60

sondrelg commented 1 year ago

No it wasn't 😅 Seems that's the default shfmt configuration. Added an .editorconfig file to revert to spaces.

sondrelg commented 1 year ago

Want to take another look at this @miigotu? 🙂 In particular, do you agree with the set -eo pipefail addition? I think having the action fail when there's an issue in the shell script seems appropriate.

miigotu commented 1 year ago

I'll go over the whole thing when I get home, but absolutely -eo pipefail is necessary. If you set the indent spaces to 2 it will keep the spacing with the 2 convention commonly used for bash which helps with reading when you have deeper nesting down the road.

It would also make the changes in this PR more apparent.