Skip to content

Let shellcheck handle good vs bad shell types #339

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

Merged
merged 1 commit into from
Aug 16, 2023

Conversation

mmlb
Copy link
Contributor

@mmlb mmlb commented Aug 15, 2023

This matches official shellcheck pre-commit hook1. This works because shellcheck will silently skip "badShells" so there isn't a good reason to do this here. The old setup was actually causing pre-commit to skip shell scripts ending with .sh since both types and types_or were specified but the identified tags resulted in an empty set since shell is not in types_or, nor is sh returned from identification:

$ cat t.sh

echo hi
$ nix-shell -p python3Packages.identify --run 'identify-cli t.sh'
["file", "non-executable", "shell", "text"]

An argument could be had that 'sh' should have shown up due to the extension similar to other shells but I still believe we should merge this and match upstream.

This matches official shellcheck pre-commit hook at [1]. This works because
shellcheck will silently skip "badShells" so there isn't a good reason to
do this here. The old setup was actually causing pre-commit to skip shell
scripts ending with .sh since both types *and* types_or were specified but
the identified tags resulted in an empty set:

$ cat t.sh

echo hi
$ nix-shell -p python3Packages.identify --run 'identify-cli t.sh'
["file", "non-executable", "shell", "text"]

An argument could be had that 'sh' should have shown up due to the extension similar to other shells but I still believe we should merge this and match upstream.

1: https://github.com/koalaman/shellcheck-precommit/blob/v0.9.0/.pre-commit-hooks.yaml#L7
@mmlb
Copy link
Contributor Author

mmlb commented Aug 15, 2023

Originally introduced in #35

@mmlb
Copy link
Contributor Author

mmlb commented Aug 16, 2023

Pinging @domenkozar

@domenkozar
Copy link
Member

@roberth

@roberth
Copy link
Contributor

roberth commented Aug 16, 2023

shellcheck will silently skip

This is a life saver! Thank you for finding this out!

Sounds good to me.

@domenkozar domenkozar merged commit 3e3d45c into cachix:master Aug 16, 2023
@domenkozar
Copy link
Member

Thank you!

@mmlb
Copy link
Contributor Author

mmlb commented Aug 16, 2023

NP 🎉

@mmlb mmlb deleted the fix-shellcheck branch August 16, 2023 19:22
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.

3 participants