[RFC] Automatically pushing format updates to pull requests

[DRMacIver]

Particularly now that we have auto cancelling for the main tests when the format job fails I've been finding the format CI almost maximally annoying.

I still want the autoformatting to run, but it would be really nice if it didn't fail the build and instead just pushed the updates to the branch.

I know some people had strong feelings against this, so I thought I'd raise the issue for discussion. Thoughts?

[alexwlchan]

I think I put the original idea in your head, so I’m a thorough 👍.

Two notes:

• I put the format job in its own build stage, and have it fails if it pushes a commit – so I spin up the smallest number of build workers if there are format changes.
• I suspect you’re the person who would get the most benefit from this. A conservative approach would be to skip auto formatting on every branch that doesn’t start DRMacIver/, which avoids touching other people’s PRs, and which I can’t imagine any objection to.

[alexwlchan]

Oh, and make sure your autoformat task doesn’t commit your SSH keys. 😄

https://alexwlchan.net/2018/07/leaking-my-ssh-keys/

[DRMacIver]

Yeah adding the branch name check is a good idea if nobody else likes this feature.

[Zac-HD]

Contra Alex, I would not split formatting to a separate stage because of the reduced throughput in all other circumstances.

More generally, why not just use a pre-commit hook to run the formatter? If it's not stable enough, should we just adopt Black and be done with it? That said, I'm perfectly happy to add 'push format changes to DRMacIver/* branches' logic to our build and leave the rest to later.

[alexwlchan]

@Zac-HD Depends how you do the build stages.

Travis build stages can condition on the branch name – including regexes. If we only want autoformatting on David’s branches, we could set up a separate stage that only runs on those branches, and is skipped entirely everywhere else. David gets fast formatting, everybody else gets fast builds.

[Zac-HD]

Oooh, neat 👍

[DRMacIver]

It could be made to work as a pre-pull hook (it's too slow for pre-commit), but TBH I'm really inclined to just turn it on for everyone than I am to use branch based filtering, unless someone gives me a really strong argument to the contrary. The upsides seem large and the downsides seem minor.

Based on what I've seen people seem really unwilling to run our build scripts locally (we should probably do some UX work on that, but that's a separate issue), so you end up with a bunch of PRs stalled with formatting errors.

[Zac-HD]

IMO the main downside is that pushing to someone else's branch without warning is a breach of normal contribution etiquette, and would be a serious turn-off to some contributors. (iirc we ran into this a few weeks ago, but I can't find it now)

Based on what I've seen people seem really unwilling to run our build scripts locally

"They don't work on Windows" means I'm not so much unwilling as unable to run locally without rebooting every time I want to work on Hypothesis 😢

[kxepal]

I think it would be better to setup https://github.com/pre-commit/pre-commit and set hook about format check. Autoformatting without user review may be not good idea because of produced WTFs.

[alexwlchan]

Another approach: use the TRAVIS_PULL_REQUEST_SLUG variable, which works as follows:

TRAVIS_PULL_REQUEST_SLUG:
if the current job is a pull request, the slug (in the form owner_name/repo_name) of the repository from which the PR originated.

Check that the PR originates from HypothesisWorks/hypothesis, and then maintainers can opt into it if they want, but it doesn't run against external contributors' PRs (or push to their branches without warning).

The big sticking point with the Hypothesis build system is that it bootstraps an entire version of Python for me, which is great in CI but annoying if I'm doing a small patch (e.g. #1477). My local dev process usually involves a venv with a recent-ish Python 3 and pytest installed. Having to wait for a recommit hook would be an annoying slowdown.

[alexwlchan]

(Also, as I enter my fifth day without Internet at home, having to download a bunch of stuff before I can do anything is mildly annoying.)

[DRMacIver]

The big sticking point with the Hypothesis build system is that it bootstraps an entire version of Python for me, which is great in CI but annoying if I'm doing a small patch

One of the things I've been intending to change about that is that if you have an existing install of pyenv then it should use that install. Would that help for you or do you not use pyenv?

[alexwlchan]

I don’t use pyenv.

[Zac-HD]

@DRMacIver - has moving to Black helped with this? If you can't be bothered setting up a branch-specific system I'm inclined to close this issue without further action.

[Zac-HD]

Closing as inactive; there are no objections to pushing formatting fixes for users who opt-in (via e.g. branch name), but nobody who can be bothered implementing it either.