https://pantsbuild.org/ logo
Join Slack
Powered by
Anyone have any problems with me replacing the `pr...
# development
w
Anyone have any problems with me replacing the 
pre-commit
default githook and instructions at https://www.pantsbuild.org/2.18/docs/contributions/development/setting-up-pants#step-3-optional-set-up-a-git-hook with a 
pre-push
instead? From conversations in the past, we have a bit of a divisive history with the 
pre-commit
hook (whether we should even have one or let CI take the hit). I'm on team githook all day, but I personally think 
pre-commit
hooks are archaic and teams should use 
pre-push
hooks instead (why does anyone need to care that I didn't format my repo for the 30 local commits, before my 1 big push?)
➕ 4
Especially on the pants repo, where linting/formatting/testing/etc can take a really long time, 
pre-commit
makes for a bad dev experience, whereas I think 
pre-push
is a sensible middleground. There are people who push often, I suppose, and we list the githook as optional (which I also disagree with), but at the very least, I think we should rename the pre-commit
g
My argument against hooks is that they break a lot of editor tooling, scripts, etc. I often end up not running lints as much as I should because it takes too long (and I can't run all of them either way due to missing system tools)... Not being able to commit because I don't have docker e.g. installed is an annoyance.
But if we have to have a hook, running it less but doing more seems a good tradeoff for me, personally.
w
My argument against hooks is that they break a lot of editor tooling, scripts, etc
That sounds like poorly designed scripts 🙂 For instance, when I have a monorepo with them - they check where the source tree was altered, only run relevant code, check if tools are installed, etc... Annnnd, very limited use of tooling - more like "you have to have this, if you're gonna be changing this code" So, in one of my recent monorepos, you only need pants installed if you're updating backend code, but frontend code just uses 
pnpm
(which, if you're changing anything in the frontend code and don't have 
pnpm
, then that's terrifying)
e.g.
Copy code
for directory in "${FRONTEND_DIRS[@]}"; do
    if has_unpushed_changes_in "${directory}"; then
        run_pnpm_checks_in "${directory}"
    fi
done

for directory in "${BACKEND_DIRS[@]}"; do
    if has_unpushed_changes_in "${directory}"; then
        run_pants_checks_in "${directory}"
    fi
done

for directory in "${UTIL_DIRS[@]}"; do
    if has_unpushed_changes_in "${directory}"; then
        run_pants_checks_in "${directory}"
    fi
done
or whatever
g
Oh, that's not how that goes. Most of the repos I work on slap a https://pre-commit.com/ hook there with one version, lock another thing into the lockfiles, and bake a third into the CI image. 😉
➕ 1
😆 2
(I'm not saying that's how it goes in Pants, my experience is just bad enough in general to avoid them when I can.)
f
I spec'd a project at work to actually make these configurable for users. Different users have different tooling workflows and different preferences for waiting, so being able to generate hook configs per-user is useful. The basic mechanism is just assigning a 
cost
value to each check and then users have a gitignored settings file they can use to set pre-commit and pre-push cost thresholds
I haven't actually done this yet, so I don't know how it will work, but I would be interested in open-sourcing this if it proves a decent solution
👍 1
w
I'm a bit more hardline in my repos 🙂 Cleaning up stuff locally isn't an option - I don't want needless CI cost on my repos, because someone didn't want to wait 7 seconds while Pushing
Especially with iOS being 10:1 minutes 🤯
f
The pre-commit.com tool is neat for like... tiny repos. It has some real flaws though
g
I'd personally be fine with something like 
[cli.alias].run-precommit = "--changed-since=upstream/main test lint whatever"
and the hook just runs that.
Maybe that's what it does, and then doing it on pre-push is fine (and I might even try it if it's pre-push).
f
For us, anything in pants that needs to resolve a large portion of our graph can take > 1 min when Pantsd is down... so many devs that don't interact with Pants regularly really don't like that
w
With something like the pants main repo, because tests are so brutal - it's basically a transitive fix/fmt/lint (and maybe test). Takes a couple seconds on my machine - saves a cycle time of like 10 minutes via CI
f
Yeah it makes sense for the Pants main repo. Agree for favoring pre-push over pre-commit in general.
h
I'm generally against hooks that affect my local workflow, so definitely opposed to pre-commit, and would be ok with pre-push only if it's fast and stable on development platforms
➕ 2
w
only if it's fast and stable on development platforms
This is largely a function of pants though. 
pants --changed-since=origin/main fix fmt lint test
or whatever... \
p
I do not use the 
pre-commit
hook on the pants repo. I let CI take the hit. I cannot run pants when I’m developing in Termux on my phone. So, the pre-commit hook is pointless there. On my laptops, I’d be more likely to enable a 
pre-push
hook than a 
pre-commit
hook, as I need commit to be snappy. I’ll put the fix/fmt in a separate commit so that I don’t lose my train of thought working on whatever code I’m focusing on.
c
I think this probably a moot point for this repo since the hook would presumably just take the form of "run pants", but I've found "which files count as modified" for pre-push hooks very difficult to reason about in the face of merges/rebases
➕ 1
5 Views
Open in Slack
PreviousNext
Powered by