Add automatic linting on git commit/push using hooks

[magopian]

This commit was initially on #101, but was moved in a separate PR to be discussed.

The time overhead on a git commit or push is less than a second on my machine. IMHO this is negligeable compared to the benefits of never committing crap, catching obvious syntax mistakes and typos, and whitespace issues.

[magopian]

Is a rebase needed here, or is the PR going to be closed at some point (and not merged)?

I'd happily rebase if it's something you'd like to have in master, and also happy to just close if it no one wants it ;)

[jankeromnes]

Thanks a lot for implementing these hooks! However, I have a few worries about them:

• Hooking something up to git commit will slow it down, while I want to be as fast as possible.
• Automatically calling git add on fixed files might have unintended side-effects: for example, I often have a lot of unstaged changes that I don't want to commit yet or ever (even within the same file as staged changes).

Do you think these worries can be addressed? (lint-staged looks like it should take care of my second worry, however this issue seems to indicate it doesn't handle partially staged files very well).

[magopian]

Reading your comment, I feel this PR shouldn't be merged:

• if you've tried the hook and find it to slow, obviously it shouldn't be merged if your concern is to keep it fast. I didn't see much of a difference (sub 1s linting usually), but I have a fast laptop
• indeed automatically fixing files is going to be an issue to people not willing to have those files fixed automatically. It's a trivial change to make it only lint (and not fix, and not git add)

I'm closing this PR now, please reopen if you think the benefits outweigh the costs

[jankeromnes]

Thanks for the update! And thank you so much for working on this contribution in the first place 😄 linting has improved this project a lot.

I didn't see much of a difference (sub 1s linting usually), but I have a fast laptop

That doesn't sound too bad, and guess it would be ok for a push hook (I still don't like anything in microseconds slowing down my commits, but maybe I'm a little extreme with this).

It's a trivial change to make it only lint (and not fix, and not git add)

This sounds like an interesting compromise. It adds a lot of friction ("sorry, you can't push because of this problem I already know how to fix but didn't fix for you"), but it's a lot less surprising (e.g. I'd hate the hook to git add stuff I'm not finished with).

@nt1m @Coder206 and @bnjbvr, would you like Janitor to have a linting hook on git push? It could complain about code issues before you actually open (or update) a pull request, by preventing you to push to any repository.

The only issue I see with this is it makes it a bit less convenient to push work-in-progress changes to a repo (but maybe fixing code issues for these too is a good idea), and you'd have to know how to use the ignore list if you're importing third-party code. Other than that, I'd like to have a linting hook (only on git push and not on git commit, and which doesn't fix or git add stuff automatically).

[magopian]

Just a side note: you said you might have some files that are modified locally, but are not going to commit/push them (ever). In such cases, if those modified files have linting warning, you'll always have to commit/push by "forcing" (i mean by using the --no-verify flag) which might be a real pain.

[nt1m]

I agree with @jankeromnes about the WIP code part. These are changes you don't want to invest too much time in.

[Coder206]

I also agree with @jankeromnes's point of view for WIP code. I wonder if we could make it optional when pushing, say behind a flag.

[bnjbvr]

I don't mind the git push hook; generally I don't open WIP PRs, so the WIP issue wouldn't affect me.