Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

✨ add a GitHub Action for shellcheck'ing on every PR and push to master branch + shellcheck'ed script #75

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

thomasmerz
Copy link

@thomasmerz thomasmerz commented Jun 7, 2024

shellcheck was complaining:

$ docker run --rm -v "$PWD:/mnt" koalaman/shellcheck:stable -Calways -S warning lsix

In lsix line 196:
	for x in ${!MAPFILE[@]}; do
                 ^------------^ SC2068 (error): Double quote array expansions to avoid re-splitting elements.


In lsix line 208:
		(cd "$arg"; $lsix)
                 ^-------^ SC2164 (warning): Use 'cd ... || exit' or 'cd ... || return' in case cd fails.

Did you mean:
		(cd "$arg" || exit; $lsix)


In lsix line 218:
    resize="[${tilewidth}x${tileheight}]"
    ^----^ SC2034 (warning): resize appears unused. Verify use (or export if used externally).


In lsix line 237:
	while [ $# -gt 0  -a  $# -gt $goal ]; do
                          ^-- SC2166 (warning): Prefer [ p ] && [ q ] as [ p -a q ] is not well defined.

For more information:
  https://www.shellcheck.net/wiki/SC2068 -- Double quote array expansions to ...
  https://www.shellcheck.net/wiki/SC2034 -- resize appears unused. Verify use...
  https://www.shellcheck.net/wiki/SC2164 -- Use 'cd ... || exit' or 'cd ... |...

This PR fixed this and I tested it:
image

My GitHub Action's Run can be found here.

@thomasmerz
Copy link
Author

@hackerb9 , I have rebased and my PR is ready for your assessment…
It should be clear which shellcheck warnings and errors are fixed (I didn't care about infos and style) and that this PR introduces a free of charge GitHub Action that will shellcheck every push to master or any PR.

lsix Outdated Show resolved Hide resolved
lsix Outdated Show resolved Hide resolved
lsix Outdated Show resolved Hide resolved
lsix Outdated Show resolved Hide resolved
@thomasmerz thomasmerz force-pushed the shellcheck branch 2 times, most recently from 44f2b64 to 0828075 Compare June 14, 2024 09:22
@@ -191,6 +191,8 @@ main() {
readarray -t < <(printf "%s\n" "$@" | sort)

# Only show first frame of animated GIFs if filename not specified.
# SC2068 (error): Double quote array expansions to avoid re-splitting elements.
# shellcheck disable=SC2068
Copy link
Owner

Choose a reason for hiding this comment

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

Oog. That's ugly. lsix is supposed to be a simple script that people can easily hack upon without all the extra cognitive load of these sort of distractions.

How about just adding the GitHub Action so it can run as an advisory linter but leave all these shellcheck comments out of the code?

Copy link
Author

Choose a reason for hiding this comment

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

These comments are neccessary for shellcheck so it won't complain about a specific error or warning in the next line. So for bash-hackers it should not confuse or distract them - but a failing shellcheck GitHub Action might confuse 🤔
I tried to fix this as mentioned/hinted on https://github.com/koalaman/shellcheck/wiki/SC2068 - but then lsix failed doing the right thing 😞 So I left shellcheck intentionally ignore this because it works/does the right thing.

Copy link
Author

Choose a reason for hiding this comment

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

I added a comment that should help other users understand why these shellcheck-lines are neccessary. And I fixed (cd "$arg"; $lsix) to (cd "$arg" && $lsix) - Do you @hackerb9 agree to that?

lsix Show resolved Hide resolved
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