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 shellcheck linting #288

Closed

Conversation

jeffbyrnes
Copy link
Contributor

@jeffbyrnes jeffbyrnes commented Mar 13, 2019

This simply adds the linting.

Relates to #287

It may be best to also add the exceptions (or adjustments) to make this pass testing as-is, since the repo is in a usable state as-is.

@jeffbyrnes
Copy link
Contributor Author

@HaleTom perhaps you’d appreciate this 🙂

@ghthor
Copy link
Member

ghthor commented Mar 13, 2019

Hell yes! Look at all that work to do!

Do shellcheck and shfmt play nicely together?

@HaleTom
Copy link
Collaborator

HaleTom commented Mar 13, 2019

@ghthor I've not heard of shfmt before, but from a very quick inspection I see no linkage to shellcheck. I only see mention of formatting (eg whitespace), whereas shellcheck issues often affect how code executes.

I'm busy with other projects right now, but this looks pretty good overall. I fixed a whole heap in my PR, and it's good to see that not too many are left:

Lines:

  • 1 - 200: These are preamble
  • 200 - 400 The guts of the problem. Assuming 5 lines per warning, that's 40 to fix. Most appear to be adding a set of surrounding ".
  • 400 - 820: Unused aliases. These warnings could be removed en-masse with an ignore directive at the top of a file or function

I expect that a few more will appear with resolving the 4 instances of:

^-- SC2148: Tips depend on target shell and yours is unknown. Add a shebang.

@HaleTom
Copy link
Collaborator

HaleTom commented Mar 13, 2019

My 2c vote: I'm vehemently against adding ignore directives simply to get Travis to green again.

I believe it's far too likely that any ignores for the sake of the colour green will be forgotten. The squeaky wheel gets oiled. Personally, I'd vote that this PR stays open for some months rather than having a "faked" resolution.

@ghthor
Copy link
Member

ghthor commented Mar 13, 2019 via email

@HaleTom
Copy link
Collaborator

HaleTom commented Mar 13, 2019

In #287, I made two proposals:

  1. A technological one (which this PR begins to address)
  2. A process / systems systems one

I cannot see how this PR addresses suggestion (2), so I assert that the comment in the top post "Closes #287" is premature or incomplete.

I respect if the consensus is that my suggestion (2) is not wanted or warranted, but need to point out that so far it has not been addressed.

@jeffbyrnes
Copy link
Contributor Author

Updated the description to avoid closing #287, per @HaleTom pointing out that discussion of their 2nd point is still pending.

Sorry for jumping the gun; I understood you in that issue to mean either/or with your two points.

@jeffbyrnes
Copy link
Contributor Author

And yes, let’s be squeaky here & properly clean these up. I’ll add to this as I get a chance!

* origin/master:
  Avoid running ll code if its not used
@jeffbyrnes
Copy link
Contributor Author

This has clearly been overtaken, closing out!

@jeffbyrnes jeffbyrnes closed this Dec 20, 2020
@jeffbyrnes jeffbyrnes deleted the add-shellcheck-linting branch December 20, 2020 04:05
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