Skip to content

Conversation

@IsaacG
Copy link
Member

@IsaacG IsaacG commented Apr 9, 2023

  • Prefer [[ over [
  • Expand tabs to 4 spaces
  • Quote variables uniformly in "${var}" format
  • Use local for function variables
  • Prefer lowercase variable names for non-readonly variables
  • Replace let with $((

@IsaacG IsaacG added x:type/coding Write code that is not student-facing content (e.g. test-runners, generators, but not exercises) x:size/small Small amount of work x:rep/small Small amount of reputation labels Apr 9, 2023
Copy link
Member

@ee7 ee7 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Big improvement - thanks.

I've checked that, with this PR, shellcheck no longer complains:

$ git rev-parse --short HEAD
d74bb32c
$ shellcheck --version
ShellCheck - shell script analysis tool
version: 0.9.0
license: GNU General Public License, version 3
website: https://www.shellcheck.net
$ shellcheck bin/check_required_files_present bin/fetch-configlet bin/reorder-keys && echo 'success'
success

I hadn't realized that the fetch-configlet script in this repo is so outdated. I'll PR to update it. It should use this version, like the track repos do.

Edit: I've created #2259. I'm happy for us to merge this PR as-is first, then rebase #2259 on top. But feel free to drop the fetch-configlet changes from this PR if you prefer. I wouldn't bother.

I've also intended to add shellcheck to CI in this repo, but apparently I never got around to it. I have it in Exercism repos that I maintain.

Comment on lines 56 to 60
if (( missing_files > 0 )); then
exit 1
else
exit 0
fi
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is exit "$(( missing_files == 0 ))" too obscure?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd prefer (( missing_files == 0 )); exit ... but being verbose here is probably good. It's longer but less likely to give anyone pause.

@IsaacG
Copy link
Member Author

IsaacG commented Apr 10, 2023

I'm quite happy letting the other PRs land first and rebasing this.

* Prefer `[[` over `[`
* Expand tabs to 4 spaces
* Quote variables uniformly in `"${var}"` format
* Use `local` for function variables
* Prefer lowercase variable names for non-readonly variables
* Replace `let` with `$((`
* Format jq script
* Simplify logic
@IsaacG IsaacG requested review from ee7 and glennj April 11, 2023 05:33
@IsaacG
Copy link
Member Author

IsaacG commented Apr 11, 2023

Rebased and I took a second pass.

Copy link
Member

@ee7 ee7 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, with some formatting suggestions.

I've checked that shellcheck is still happy.

Non-blocking comments:

  • As a later follow-up, I would support renaming bin/check_required_files_present to bin/check-required-files-present, to match bin/reorder-keys.
  • Again as a later follow-up, I'd probably support a PR that adds a script formatting check to CI. Something like: shfmt -i 4 -ci -sr -kp. Or whatever.
  • There's maybe a slight precedent for preferring to indent bash scripts with 2 spaces: that's what the fetch-configlet script (now) uses in this repo, and in every track repo. But that script will probably be removed from this repo, and I don't mind either way.

@IsaacG IsaacG merged commit cf59350 into exercism:main Apr 12, 2023
@IsaacG IsaacG deleted the tidy_shellscripts branch April 12, 2023 15:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

x:rep/small Small amount of reputation x:size/small Small amount of work x:type/coding Write code that is not student-facing content (e.g. test-runners, generators, but not exercises)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants