Use pre-commit.ci on PRs

[JelleZijlstra]

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.

[Akuli]

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]

I don't much care for local pre-commit hooks either, but automatically fixing up PRs seems really useful.

[srittau]

I haven't looked at pre-commit.ci yet, but any improvement in this area is welcome!

[Akuli]

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]

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]

Yeah, it sounds awesome until the hook breaks and returns status 1, making git commit fail and contributing impossible.

[JelleZijlstra]

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]

Ah, so it's actually not pre-commit at all, but rather post-push. I was confused by the name. Nevermind.

[Akuli]

I can see one possible situation where this causes a problem:

1. PR author pushes code that uses lots of single quotes
2. Black changes every single quote to double quote, editing almost every line
3. PR author makes another commit that rewrites the whole PR
4. PR author pushes, gets error telling to pull first
5. PR author pulls, and gets merge conflicts on almost every line

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]

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]

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 / isort warnings is not very pleasant.

[AlexWaygood]

+1 to what @sobolevn said

[JelleZijlstra]

Then let's do it. I can try to set this up but no guarantees on when I'll have time for this.

[JelleZijlstra]

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]

neat!

just to clear a few things up above (I made pre-commit and pre-commit.ci):

I would prefer to not have anything automagically install a git hook when I run pip install -r requirements-tests-py3.txt

yeah I hate that too, some tools do this but I will never allow pre-commit to do that! setting up the local hooks is a required opt-in (via pre-commit install) so it's also obvious to opt-out (pre-commit uninstall)

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.

fortunately the client-side hooks are entirely optional and pre-commit.ci will auto-fix things for them \o/

Ah, so it's actually not pre-commit at all, but rather post-push. I was confused by the name. Nevermind.

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]

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:

• I messed up the yaml syntax initially and got a non-informative error (https://results.pre-commit.ci/run/github/89016131/1637299223.xxx9dyZ3T2y9y15z6D4RgQ; fix in JelleZijlstra/typeshed_client@0b88dd8). Not sure if it would have been possible for pre-commit to give a better error there.
• It was a bit surprising to get various files Black-formatted that previously weren't, but as I recall that's more of a Black issue and it's why we added the --force-exclude flag to Black a little while ago.

[asottile]

@JelleZijlstra

• yeah that's just a generic "could not decode yaml error" which unfortunately there's not much that can be done -- technically you shouldn't have needed any configuration as you've set all the values to the default (the ci section is entirely optional!)
• yeah the second thing is just black's force-exclude -- you could set that so pre-commit doesn't pass those filenames along to black (would slightly improve performance) but force-exclude is another solution