-
Notifications
You must be signed in to change notification settings - Fork 481
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 Shellcheck support via GitHub Actions. Fixes all violations. #101
Conversation
And here's evidence that Shellcheck fires up fine https://github.com/basecamp/omakub/actions/runs/9482343428/job/26127026453?pr=101 |
I know shellcheck likes this, but imo it isn't really needed, and imo kind of ruins the cleanness of the codebase, because the code will still work #!/usr/bin/env bash |
Explanation here: https://stackoverflow.com/a/25202821 I'd say if we want to start turning off rules that Shellcheck ships with, it's perphaps a sign that we don't want Shellcheck to begin with. |
Disagree that it ruins cleanliness - it's just one comment line in the scripts, and it ensures the scripts will work correctly (by invoking bash) even if the user has switched to a different shell after installation. |
3737a42
to
aeaf728
Compare
aeaf728
to
31b2cd2
Compare
@dhh I keep on rebasing changes (you move fast!), so in order to proceed with integrating the codebase with Shellcheck, if you're still interested with this I think we should merge ASAP. |
5b1b5e2
to
2fb955c
Compare
Hmm, looking through the list of changes, I'm not sure I'm seeing enough value here. I like the idea in theory, but the practice of the diff isn't convincing enough imo. Appreciate you investigating this though! |
@dhh Separate from the other changes, would you accept the addition of a |
# shellcheck disable=<shellcheck_code>
immediately above a line to silence violations (per https://www.shellcheck.net/wiki/Directive) since I can't test this code locally (I'm on Mac but supportive of the effort!) and don't want to break things (this should still be tested as I might have missed something).Background: #70 was reverted because the lift was too much and
master
needed to be cleaned up first.Artifact for disabled violations: