Skip to content

Conversation

@damacus
Copy link
Contributor

@damacus damacus commented Jan 10, 2024

No description provided.

Signed-off-by: Dan Webb <dan.webb@damacus.io>
@damacus
Copy link
Contributor Author

damacus commented Jan 11, 2024

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)

Signed-off-by: Dan Webb <dan.webb@damacus.io>
Signed-off-by: Dan Webb <dan.webb@damacus.io>
Signed-off-by: Dan Webb <dan.webb@damacus.io>
@Gadgetoid
Copy link
Member

Gadgetoid commented Jan 11, 2024

Oh, I fixed everything in the linked PR against the upstream boilerplate here - pimoroni/boilerplate-python@5e294af

I have a PR bringing those fixes into this repo, which should stop shellcheck complaining here - #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
Copy link
Contributor Author

damacus commented Jan 11, 2024

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
Copy link
Contributor Author

damacus commented Jan 11, 2024

Closing in favour of #42

@damacus damacus closed this Jan 11, 2024
@damacus damacus deleted the shellcheck branch January 11, 2024 13:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants