pimoroni / grow-python

Python libs for controlling the Grow HATs
MIT License
55 stars 44 forks source link

Add Shellcheck action #41

Closed damacus closed 6 months ago

damacus commented 6 months ago

It's worth discussing what you do and don't want to follow in shellcheck.

Some of this makes a lot of sense (i.e. using the new format for sub-shelling), and some of it makes less sense like when you're checking the value of a variable.

I'm also not sure what the goal is with this bit of code:

PACKAGES="${PACKAGES[@]}"

We get the following shellcheck error:

Assigning an array to a string! Assign as array, or use * instead of @ to concatenate.shellcheck[SC2124](https://www.shellcheck.net/wiki/SC2124)

Gadgetoid commented 6 months ago

Oh, I fixed everything in the linked PR against the upstream boilerplate here - https://github.com/pimoroni/boilerplate-python/pull/13/commits/5e294af079b65f79c169023e437276794c4af59d

I have a PR bringing those fixes into this repo, which should stop shellcheck complaining here - https://github.com/pimoroni/grow-python/pull/42

Sorry I didn't mean to prompt a duplication of effort here!

Edit: Note: I think my array access code was just completely wrong and PACKAGES="${PACKAGES[@]}" was just bad practise, since it's overwriting the original array with a string value which would never fly in a strongly typed environment.

I'm generally happy to try and fix all shellcheck issues (as I did in the linked PRs) and, on a case by case basis, add ignore comments in for the ones that really, definitely don't make sense. I think catching style issues and warnings will probably fend off bugs in the long term, even though we might not agree with everything it suggests.

I don't purport to be super great at shell, so having some training wheels on will be very useful 😆

damacus commented 6 months ago

Good to know someone else does arrays as wrong as I do in bash 😆

Yeah, I've used shell check to make me better at Bash over the past few years. The syntax you've been using here is super super common. So I'm not surprised at any of the comments shellcheck made

damacus commented 6 months ago

Closing in favour of #42