Repository navigation
Use pre-commit.ci on PRs #5552
Description
https://pre-commit.ci/ would allow us to automatically fix Black and isort formatting on PRs. Currently I see a lot of PRs where people have to manually fix up formatting after opening a PR, so automating this process would be useful.
Activity
Akuli commentedon May 29, 2021
I would prefer to not have anything automagically install a git hook when I run
pip install -r requirements-tests-py3.txt. This seems to be the purpose of pre-commit.ci? We already have a pre-commit hook that applies black and isort (but apparently nobody uses).JelleZijlstra commentedon May 29, 2021
I don't much care for local pre-commit hooks either, but automatically fixing up PRs seems really useful.
srittau commentedon May 29, 2021
I haven't looked at pre-commit.ci yet, but any improvement in this area is welcome!
Akuli commentedon Jun 6, 2021
My biggest fear with this is a contributor getting an error from a pre-commit hook that they don't know how to disable/fix. With the current pre-commit hook (a script, not the project named pre-commit), you have to move it to the right place, so you automatically learn how to disable it in case it doesn't work.
#5188 is an example of a pull request where the contributor couldn't get black to work, but was able to contribute anyway. This wouldn't be possible of a pre-commit hook ran black.
JelleZijlstra commentedon Jun 6, 2021
The idea is that the pre-commit hook would automatically run Black on the PR and do the formatting for them, so they don't ever need to do formatting themselves.
Akuli commentedon Jun 6, 2021
Yeah, it sounds awesome until the hook breaks and returns status 1, making
git commitfail and contributing impossible.JelleZijlstra commentedon Jun 6, 2021
Is that how it works? My understanding is that people just write code locally with no interaction with pre-commit.ci, and then when an PR is opened it runs just like GitHub Actions.
Akuli commentedon Jun 6, 2021
Ah, so it's actually not pre-commit at all, but rather post-push. I was confused by the name. Nevermind.
Akuli commentedon Jun 6, 2021
I can see one possible situation where this causes a problem:
With manual blacking, this kind of merge conflict problem would never occur, for two reasons: blacking is done in the same place as other changes, and it's sometimes done toward the end of the PR when other things are already working.
A similar situation is possible with the "Apply suggestion" button, but in much smaller scale: a suggestion changes only a few lines, whereas blacking single-quoted code changes basically everything.
hauntsaninja commentedon Jun 6, 2021
Still seems worth doing to me / on net a win for contributor experience. For the cases you mentioned, one could revert the pre-commit commit locally, cherry pick and push (and let pre-commit reformat). If it comes up and a contributor gets stuck, we'll be here to help out (and if we find it's not a net win we can always remove it).
Very much in favour of not requiring pre-commit use locally, though!
sobolevn commentedon Nov 18, 2021
This would be a huge win for me. I sometimes prefer to program on GitHub (yes, this is strange, I know) (but this allows me to quickly change contexts, especially for typeshed where most of the PRs only touch a single file). Manually fixing
black/isortwarnings is not very pleasant.AlexWaygood commentedon Nov 18, 2021
+1 to what @sobolevn said
JelleZijlstra commentedon Nov 18, 2021
Then let's do it. I can try to set this up but no guarantees on when I'll have time for this.
JelleZijlstra commentedon Nov 19, 2021
I just set it up on my own project (JelleZijlstra/typeshed_client#29) and it was pretty easy. Sending a PR for typeshed soon.
asottile commentedon Nov 19, 2021
neat!
just to clear a few things up above (I made
pre-commitandpre-commit.ci):yeah I hate that too, some tools do this but I will never allow
pre-committo do that! setting up the local hooks is a required opt-in (viapre-commit install) so it's also obvious to opt-out (pre-commit uninstall)fortunately the client-side hooks are entirely optional and pre-commit.ci will auto-fix things for them \o/
yeah admittedly the naming for the original tool kinda isn't the best -- when I created it (pre-commit.com) I never expected it to be as popular as it is or have many additional use cases as it now does -- and the ci system hooks onto the branding of the tool so it's also named "pre-commit"
JelleZijlstra commentedon Nov 19, 2021
Thanks @asottile for building this tool and for the clarifications here!
It was a pretty smooth experience setting this up, but two pieces of feedback:
--force-excludeflag to Black a little while ago.asottile commentedon Nov 19, 2021
@JelleZijlstra
cisection is entirely optional!)black's force-exclude -- you could set that so pre-commit doesn't pass those filenames along toblack(would slightly improve performance) butforce-excludeis another solution