[WIP] adding pre-commit CI and JuliaFormatter

[Moelf]

fix #47052

waiting admin of this repo:

• enable pre-commit ci for this repo: https://pre-commit.ci/
• check what other hooks we may want before JuliaFormatter can replace them: https://pre-commit.com/hooks.html

Waiting for upstream:

• error formatting stdlib of Julia domluna/JuliaFormatter.jl#644
• Empty style for strictly opt-in functionality domluna/JuliaFormatter.jl#561

Test drive:

• run JuliaFormatter on Base64 stdlib #47151

[DilumAluthge]

1. Instead of splitting our CI between Buildkite and GitHub Actions, I'd like us to keep everything on Buildkite, which will make things easier to maintain.
2. This workflow will not work on PRs from forks.

Also see my inline comments.

[Moelf]

@DilumAluthge

To make it easier for the PR author to identify formatting issues, before we exit, we should print the list of file names and line numbers where formatting issues are present (which is also what the existing whitespace check does).

well, this would make this automation doing the opposite of what it should be doing then, the point of auto-formattingios is so that people making PR don't have to worry about being format-perfect and maintainer don't have to do bunch of style correct and trigger a bunch of useless CIs.

If our formatting CI merely shows what's the problem and failing, you essentially give PR author more burden:

• have to install JuliaFormatter
• have to figure out the exact style we use and run it

[Moelf]

Instead of splitting our CI between Buildkite and GitHub Actions, I'd like us to keep everything on Buildkite, which will make things easier to maintain.

I think the opposite, the Github CI are natural if you're dealing with source file that requires github mechanics to modify anyway, BuildKite is a heavy cannon, and I'm not sure setting up "write access" from a Builkite hook or something is exactly secure and easy to maintain

But if you can point me to how buildkite pre-commit CI works (or whatever they call it), I'd happy to take a look, but a quick google didn't yield anything that's ready-made

[DilumAluthge]

you essentially give PR author more burden:

• have to install JuliaFormatter
• have to figure out the exact style we use and run it

I think the right approach here is to have a Makefile target of the form make format that would do all of that automatically.

[Moelf]

ok, new plan after talking to @DilumAluthge on Slack, the motivation for this design is to reduce potential security risk:

1. make a bot, and it doesn't need write access to this repo
2. in an untrusted Buildkite job, run whatever formatting we need
3. in a separate, trusted Buildkite job, download tarball and use bot's access token to leave inline commts for fixing styles

[Saransh-cpp]

Cross-linking the discussion available on FluxML's PR here for help - FluxML/Flux.jl#2074

[t-bltg]

Are the annotations left by reviewdog/action-suggester really helpful ? IMO, they just pollute the Files changed view by adding tons of annotations. I know they can be disabled (manual intervention needed - cumbersome), but they tend to distract attention. I think that the failing JuliaFormatter check is enough (we don't want contributors to fix this by hand as reviewdog suggests (wrong signal), but run the formatter in the repo instead).

[Moelf]

They don't pollute file changes more than the original PR, in the ideal case it pushes to the PR branch

Failing a formatting test is a step in wrong direction, now you have something that PR author needs to download and setup in order to make PR, super annoying

[rikhuijzer]

PRs with many comments, like for example #43990, would become even more unwieldy though.

[Moelf]

which is why I always wanted it to be directly pushing to fork branch

[Moelf]

I mean idk, it seems people think nothing is better than anything so maybe I will just drop this

[maleadt]

Is the goal to format the entire codebase, or parts of the codebase, in their entirety? I always liked how git clang-format work, only formatting the changes you make, which is far less intrusive. I guess if we'll be using a bot that suggests changes, it'll only apply to changes made in a PR?

[Moelf]

@maleadt for PR yes; but that's also a pre-condition for the entire code base to have one formatting style (if we can agree on a minimal set of formatting rules).

Even if we format the entire codebase at some point, if individual PRs, later on, don't enforce the same formatting rule, it's useless as it will just break the codebase piece by piece.

idk about formatting the entire codebase, but at least we should format future PRs in someway as a starter

[LilithHafner]

I prefer a bot automatically making inline suggestions over automatically pushing to a branch because automatic pushes can cause unexpected merge conflicts when a PR author makes the reasonable assumption that they are the only one pushing to the PR branch.

I think that automatic inline suggestions for some small subset of formatting decisions is strictly better than the status quo because it reduces the burden on reviewers to make those suggestions and gives authors prompt suggestions for how to improve formatting that are acceptable via GUI, and, if need be, the suggestions can simply be marked as resolved ad ignored in which case they don't take up much space.

[IanButterworth]

What about an action that runs every night on master and creates a PR for review & tests. The first one might be a little painful for open PRs but after that it should be manageable.

[Moelf]

I think that automatic inline suggestions for some small subset of formatting decisions is strictly better than the status quo

yeah that's what we're moving towards and the problem now is we can't white-list what rules we want, there's a set of rules can't be disabled in JuliaFormatter.jl right now

What about an action that runs every night on master and creates a PR for review & tests.

adding burden rather than reducing, I don't like this idea.