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

Check scripts with shellcheck before accepting new commands #781

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

Conversation

pabs3
Copy link
Contributor

@pabs3 pabs3 commented Sep 11, 2019

No description provided.

@pabs3 pabs3 force-pushed the shellcheck branch 5 times, most recently from 38d06ba to 7825251 Compare September 11, 2019 07:22
@spacewander
Copy link
Collaborator

shellcheck is great. But to make the shellcheck happy with the existing scripts, we have to do a lot of things.

If we can not distinguish the new change and the existing code in a script, the contributor have to upgrade the existing code to pass the CI. This burden makes the contribution harder, especially when the contributor only wants to add a simple option or correct typo.

As the gatekeeper, I have already used shellcheck to verify each change, and ignored some warning manually. Sometimes SC2086: Double quote to prevent globbing and word splitting. is too strict.

@pabs3
Copy link
Contributor Author

pabs3 commented Sep 12, 2019 via email

@pabs3 pabs3 force-pushed the shellcheck branch 15 times, most recently from d7371f4 to 1409c3c Compare September 19, 2019 06:43
@pabs3
Copy link
Contributor Author

pabs3 commented Sep 19, 2019 via email

Only check scripts that have been added in the current pull request.

Do not check them on macOS as homebrew is slow to install shellcheck and
the macOS cut command does not support an empty -d argument.

FIXME: PORT THIS TO GITHUB ACTIONS FROM TRAVIS
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