Add shellcheck linting - Githubissues

scmbreeze / scm_breeze

Adds numbered shortcuts to the output git status, and much more

https://madebynathan.com/2011/10/19/git-shortcuts-like-youve-never-seen-before/

MIT License

2.82k stars 192 forks source link

Add shellcheck linting #288

Closed jeffbyrnes closed 3 years ago

jeffbyrnes commented 5 years ago

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 commented 5 years ago

@HaleTom perhaps you’d appreciate this 🙂

ghthor commented 5 years ago

Hell yes! Look at all that work to do!

Do shellcheck and shfmt play nicely together?

HaleTom commented 5 years ago

@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 commented 5 years ago

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 commented 5 years ago

I agree with you @haletom about leaving this squeaky.

On Wed, Mar 13, 2019 at 5:16 AM Tom Hale notifications@github.com wrote:

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.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/scmbreeze/scm_breeze/pull/288#issuecomment-472340737, or mute the thread https://github.com/notifications/unsubscribe-auth/AAJyKqHfppUSO7B8xZbnnkSk8YQzaKpuks5vWMIGgaJpZM4bsJhi .

HaleTom commented 5 years ago

In #287, I made two proposals:

A technological one (which this PR begins to address)
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 commented 5 years ago

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 commented 5 years ago

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

jeffbyrnes commented 3 years ago

This has clearly been overtaken, closing out!