https://pantsbuild.org/ logo
Join Slack
Powered by
Thread re removing our git hooks:
# development
h
Thread re removing our git hooks:
I am removing our prepare-commit-msg hook (the one that added `[ci skip-rust]`/`[ci skip-build-wheels]` to commit messages, since we now detect rust/wheel changes in a more principled way (post https://github.com/pantsbuild/pants/pull/17224)
🎉 1
That leaves us with one hook left, the pre-commit hook, which runs a bunch of fairly heavyweight stuff: lint, fmt, check and more
Personally I find that intensely annoying, since I commit and push often to save my work
So I just don’t install that hook
and I’m wondering if we can just do without it
and treat devs like the adults they are, who can run checks as needed as part of their workflow
And if they don’t, CI will catch them out
So, I’m proposing to get rid of our git hooks, and simplify things a little
f
I've long since disabled hooks in my clone of the repo ...
rust builds take too long and I too commit frequently
so +1 from me
h
By vote of emoji, all in favor/opposed to getting rid of our git hooks?
👎 4
👍 4
w
What's the incentive to remove pre-commit hooks, when they're optional to add in the first place? I would prefer they were pre-push hooks myself - as I don't really care whether the 10 in-between commits pass or not. But at the same time, I don't have to enable them in the first place
I think they also are a great check for novice Pants contributors, and progressively lose value over time/experience. But again... Optional to add
h
We sort-of imply that to develop on Pants you’re expected to install these githooks
and I am allergic to imposing process on other devs at this level
w
and I am allergic to imposing process on other devs at this level
And I'm allergic to re-running CI because of a misplaced newline 🙂 I think maybe the wording could be modified to pointing out that a githook is available, but I think the hook file itself still has value being a part of the repo
h
I never run them because they take so long. I'm vaguely in favor of what SJ proposes, or somehow making them faster
w
From an analytics POV, is there any metric that can give us insight into how many CI runs fail due to something that pre-commit could have caught? One would assume this should be zero-ish right now?
h
how often does the lint job fail
w
lint and check, correct?
h
yeah
w
Well, found 2-3 lint failures in the first 2 pages of workflows, but not sure how to do it programmatically - and also to remove dependabot from the analysis. 🤷
w
fwiw: you can run the hooks in debug mode:
Copy code
MODE=debug git commit ...
… or skip them only in some cases:
Copy code
git commit ... -n
… so i have them enabled, and skip them when i suspect it is safe
so i would vote against deleting them. people can continue to not install them if they’d like.
👍 4
h
It just seems arbitrary. Why lint and check, but not tests?
w
In my repos, it's 
lint check test
- but in the mainline repo, 
test
is comically slow, while 
lint
is pretty quick, and 
check
isn't too bad. Also, in this repo, because CI does the full matrix test and has multiple platforms, passing locally doesn't really give the full picture on whether CI would pass (though, it's quite reasonably accurate). I still try to 
test
when I can remember pre-push, but I'm also semi-reliant on what CI says at the end. 🤷‍♂️
Open in Slack
PreviousNext
Powered by