search

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.83k stars 193 forks source link

Project coding standards #287

Open HaleTom opened 5 years ago

HaleTom commented 5 years ago

Dear fellow maintainers,

You may remember me as the guy who dropped a significant PR a few months ago.

After fixing many quoting issues, I saw someone (I believe it was the original author, but could be wrong) making some commits with introduced another one or two of the same class of bug that I'd just spend a long time fixing.

(Please always quote $@ unless you have an explicit reason not to, including the non-usage of arrays.)

I was thoroughly discouraged, and haven't contributed since (saving issues directly related to the code that I contributed.)

So, I'd like to raise an observation, and make a proposal.

My observation (right or wrong, please correct me) is that this project seems to have started as a convenience script, and the original author adds things without a code review process.

This code base seems to have new features added to do nifty timesaving tricks, but lacks an emphasis on correctness and robustness, prioritises features over quality, and lacks edge- and corner-case tests.

sh and its derivatives are arguably poorly designed languages and edge case foot-shooting is common.

I propose:

  1. To catch bugs, that this project be shellcheck clean.

    Yes, sometimes shellcheck is annoying and gets it wrong. But it forces coders (via an ignore directive) to explicitly say "yes, I've read the cautions and really do know what I'm doing".... or (more commonly) to say: "damn, that's an edge case" and fix what will likely result in unexpected behaviour down the track.

  2. That there be a review process for contributed code.

jeffbyrnes commented 5 years ago

@HaleTom I’m a big fan of spellcheck, and I’d be on board with adhering to it, with specific carveouts being exactly that: clearly commented, per-exception carveouts (which I think it allows? I’m assuming that based on the behavior of other linters).

HaleTom commented 5 years ago

@jeffbyrnes Great work on #288 w.r.t suggestion (1).

Yes, "Shaddup, I know what I'm doing" is definitely supported by shellcheck. :)

I would appreciate thoughts on suggestion (2) from anyone.

ndbroadbent commented 5 years ago

Hi @HaleTom, I’m really sorry that I introduced those bugs! Sorry, I just saw this issue now. (I stopped watching the repo and turned off notifications since I got a bit burned out from responding to issues.)

I’ll definitely stop pushing any more code directly to master, and will open a PR next time to get some feedback first. (Although I’m not planning to work on any more changes.)

shellcheck sounds like an awesome idea, and that will be great to add to the CI build.

Sorry again, that won’t happen again! And thanks a lot for contributing those fixes!