Should PR builds pass with formatting errors?

[Zac-HD]

We use several tools that apply some kind of canonicalisation to our source code: autoflake, pyupgrade, isort, and of course black. We also want to enforce an invariant that whatever is on master is always a fixpoint of these tools.

Currently, we do this by enforcing that running them does not produce a diff on each pull request. This obviously works, but it's often a frustrating experience for new contributors as they discover a series of automatically-fixable issues via failing CI builds.

After recent discussions about trio adopting bors-based tooling, I realized that our release-everything approach makes the following workflow possible:

Do not fail a PR build for anything that can be fixed by a tool.
Instead, apply them in the version-bump commit.

+ faster and nicer experience for new contributors
- line numbers in tracebacks or lint errors may not match local positions
- needs new logic to apply/commit/push fixes for non-released PRs (e.g. test-only changes)

My specific proposal is to continue enforcing black (which is easy to install and run), but move to automatically applying the other tools - I think that gets us most of the upsides and minimal downside.

Thoughts?

[fenollp]

I think format enforcing must be part of CI. But there must be an easy way (some one-liner) to fix the formatting and re-push easily. The only pain point is a tooling issue!
Something like https://github.com/jbbarth/docker-black can do the job ;)

[Zac-HD]

Pushing changes to a contributor's branch is an attractive idea, but can cause immensely frustrating merge conflicts for new developers. The idea here is that we don't need to enforce changes that we can apply automatically after merging!

[Zalathar]

In my experience, the biggest pain point is not that formatting fails the build, but that formatting errors prevent the rest of the test suite from running at all.

From that perspective, a much less ambitious approach would be to remove the dependency/phasing relationship between format-checking and the main test suite.

If I know that everything other than formatting is OK, then I'm happy to run the formatter myself and re-push for one final test run. (Though being able to press a button and have a bot do that for me would be a nice convenience.)

Similarly, for new contributors, this would at least remove the frustrating step of having to satisfy the formatter before getting any useful feedback on your change's functionality. Once the PR is in a good-modulo-formatting state, then it's easier to muster the motivation to do the boring final step.

[DRMacIver]

Do not fail a PR build for anything that can be fixed by a tool.
Instead, apply them in the version-bump commit.

I think the problem with this plan is that there's a bunch of stuff like linting where we logically want to run it after the formatting. If we format and this makes changes but then fail linting, you're now emitting errors on a version of the source the user did not write.

This a) Makes it so the user really needs to run the formatting job locally anyway and b) Is quite confusing.

But there must be an easy way (some one-liner) to fix the formatting and re-push easily. The only pain point is a tooling issue!

There is and always has been! The CI tooling is all designed to be run wherever, CI or not. ./build.sh format will do exactly the same as the CI formatting job. The only pain point here is that the first time you run it will install a bunch of the build tooling.

Granted that doesn't automatically repush, but ./build.sh format && git commit -a -m "Reformat" && git push is technically still one line. 😄

In my experience, the biggest pain point is not that formatting fails the build, but that formatting errors prevent the rest of the test suite from running at all.

I think the reasoning on having formatting run first and fail the build immediately was that we were having quite high build contention and a formatting fail would indicate that someone was likely to repush shortly and as a result it was pointless build contention.

I think that's still true (but mitigated with the move to Azure) but am open to the idea that the cost to new contributors is a greater burden than it's worth.

[Zac-HD]

I agree that lint errors against formatted source could be confusing - this is my own biggest concern. Possible mitigations include "still enforce Black", to reduce the diff, or maybe something fancier.

Our build / CI tooling doesn't run on Windows (:sob:), which is disproportionally used by new contributors.

Re: build contention - I'd be happy to remove the staging on Azure, but I think Travis probably needs it. More reason to finish migrating soon!

[DRMacIver]

Our build / CI tooling doesn't run on Windows (😭), which is disproportionally used by new contributors.

It's not an ideal solution I'll grant, but every modern windows ships with a Linux distribution that can run it.

We could also write a windows batch script wrapper that just runs the build tooling in docker and make docker a dev-on-windows requirement.

[Zac-HD]

I agree that there are workarounds, but the easiest for beginners continues to be "pip install one or two tools, then push and let CI tell you what happened". So unless we can make our tooling universally easier than that, it would be nice to make that experience more friendly.

[Zac-HD]

Having hammered out a quick spike... no, making it easier to run our build system on Windows would be a better use of time.

We've also - on Pipelines - allowed other tests to run despite lint errors, so you don't have to get formatting right before correctness will be checked.