Skip to content

Shellcheck part 1: bin scripts and non-example exercise files#784

Merged
glennj merged 4 commits into
exercism:mainfrom
glennj:shellcheck
Jun 15, 2026
Merged

Shellcheck part 1: bin scripts and non-example exercise files#784
glennj merged 4 commits into
exercism:mainfrom
glennj:shellcheck

Conversation

@glennj

@glennj glennj commented Jun 15, 2026

Copy link
Copy Markdown
Contributor

Ref: issue #783

This PR will satisfy

find exercises/practice bin \
    \( -name '*.sh' -o -name '*.bash' \) \
    -not \( -name example.sh -o -path exercises/practice/markdown/markdown.sh \) \
    -print0 \
| xargs -0 shellcheck

I'd recommend reviewing by commit: the last commit is just copying one bats-extra.bash file to all the rest of the exercises.

@github-actions

Copy link
Copy Markdown
Contributor

This PR touches files which potentially affect the outcome of the tests of an exercise. This will cause all students' solutions to affected exercises to be re-tested.

If this PR does not affect the result of the test (or, for example, adds an edge case that is not worth rerunning all tests for), please add the following to the merge-commit message which will stops student's tests from re-running. Please copy-paste to avoid typos.

[no important files changed]

For more information, refer to the documentation. If you are unsure whether to add the message or not, please ping @exercism/maintainers-admin in a comment. Thank you!

Comment on lines +20 to +21
local list1=()
local list2=()

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The test variables are now localized: shell check was complaining about result being used as a scalar after previously being an array.

@glennj glennj requested a review from a team June 15, 2026 12:46
Comment thread exercises/practice/acronym/bats-extra.bash Outdated
Comment on lines +372 to +374
# shellcheck disable=SC2319 # (warning) `$?` in a conditional expression: https://github.com/koalaman/shellcheck/wiki/SC2319
if [[ '' =~ $expected ]] || (( $? == 2 )); then
# the regex matches an empty string or it is syntactically incorrect.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I suspect matching the empty string is not intentional. We could allow an empty match and/or fail explicitly on an empty match.

[[ '' =~ $expected ]]
match=$?
msg=""
if (( match == 0 )); then
  msg="Regex matches an empty string"
elif (( match == 2 )); then
  msg="Invalid regex"
fi
if [[ -n $msg ]]; then
  echo "${msg}: ${expected@Q}"
  ...

@glennj glennj Jun 15, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I suspect matching the empty string is not intentional.

Betcha it is.

I don't really want to diverge the code we took from bats-core/bats-assert.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We could add an ignore for these files. There's an ignore option in the workflow.

@glennj glennj merged commit 231e780 into exercism:main Jun 15, 2026
3 checks passed
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