https://pantsbuild.org/ logo
#development
Title
# development
h

happy-kitchen-89482

10/15/2022, 12:27 AM
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

fast-nail-55400

10/15/2022, 12:33 AM
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

happy-kitchen-89482

10/15/2022, 12:34 AM
By vote of emoji, all in favor/opposed to getting rid of our git hooks?
👎 4
👍 4
w

wide-midnight-78598

10/15/2022, 12:40 AM
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

happy-kitchen-89482

10/15/2022, 1:05 AM
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

wide-midnight-78598

10/15/2022, 1:16 AM
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

hundreds-father-404

10/15/2022, 2:19 AM
I never run them because they take so long. I'm vaguely in favor of what SJ proposes, or somehow making them faster
w

wide-midnight-78598

10/15/2022, 2:31 AM
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

hundreds-father-404

10/15/2022, 2:42 AM
how often does the lint job fail
w

wide-midnight-78598

10/15/2022, 2:44 AM
lint and check, correct?
h

hundreds-father-404

10/15/2022, 2:51 AM
yeah
w

wide-midnight-78598

10/15/2022, 3:08 AM
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

witty-crayon-22786

10/15/2022, 3:11 AM
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

happy-kitchen-89482

10/17/2022, 6:16 AM
It just seems arbitrary. Why lint and check, but not tests?
w

wide-midnight-78598

10/17/2022, 11:58 AM
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. 🤷‍♂️