Please add automatic black formatting to workflow

[RubendeBruin]

Black is nice, but it would be even better if it was automatically applied in PRs. Or via a black-bot. Now to be able to make a PR someone needs to have Black installed.

[Lucas-C]

Black is nice, but it would be even better if it was automatically applied in PRs.

Agreed, nice suggestion!
I may not have the time to add this to our GitHub Actions workflow in the following days,
but I'd be happy to merge a PR that would introduce auto-running black on PRs.

[gmischler]

Automating tedious stuff as much as possible is certainly desirable.

The optimal place to automatically format the code seems to be as a local check-in hook. This makes sure that what you see in your editor is the same thing as placed in the repository. Given the largish number of dependencies you need for local testing anyway, I'm not sure if "I don't want black on my machine" is a good argument against that solution.

A bad place in the middle is the current one, where in a PR (even a draft) no functional tests are performed on github when the formatting isn't up to snuff. This is rather inconvenient while still working on a PR and the functional feedback on the different platforms would be helpful, but manually reformatting before each commit is time consuming and annoying.

And at the other end of the spectrum we could have black applied only at the very end when the PR is actually merged. This would make the formatting irrelevant during development, but the official repository still only contains well formatted code.

Unfortunately, this still leaves pylint in play, which is even more annoying because it takes so long to run.

To solve possibly both annoyances in one go, I have a different suggestion:
Is it possible to stagger the jobs in the PR pipeline?
If so, then we could first run all the testing on all the platform/version combinations, and only after those have successfully passed run another seperate job that does the pylint/black combo. This provides the full value of the functional test results unhindered by formatting issues.
Applying the black changes within the pipeline just before merging may still be possible.

[Lucas-C]

If so, then we could first run all the testing on all the platform/version combinations, and only after those have successfully passed run another seperate job that does the pylint/black combo. This provides the full value of the functional test results unhindered by formatting issues.

Actually, I have an unfinished PR open on th Pylint repository,
and they use a solution similar to this: pre-commit.ci

If the code in a PR would require fixups by pre-commit hooks,
an extra commit is added by a bot with those fixups:
https://github.com/pylint-dev/pylint/pull/8663/commits

I'm very familiar with pre-commit hooks, I even maintain some: https://github.com/Lucas-C/pre-commit-hooks
They are a very handy tool, as long as the checks do not take too long to run.

Maybe we could try experimenting with this solution?

[RubendeBruin]

For me it would be ideal if:

• the tests

• black

• pylint
  would all run in parallel and regardless of the outcome of the others

• black would be available via a bot. I think I encountered that somewhere (VTK maybe?). There you could do "please black-format" or something and then the bot would create a new commit with all the code black formatted.

[prateek-dagar]

Hi @Lucas-C ,

Since the repository already has .pre-commit-config.yaml configured, we could easily automate this by enabling pre-commit.ci.

I would be happy to submit a quick PR adding the ci: configuration block to .pre-commit-config.yaml if you'd like to go ahead with this.

Here is the proposed addition:

ci:
  autofix_commit_msg: '[pre-commit.ci] auto fixes from pre-commit.com hooks'
  autoupdate_commit_msg: '[pre-commit.ci] pre-commit autoupdate'
  autoupdate_schedule: weekly
  skip: [pylint]

[andersonhc]

@prateek-dagar

That sounds like a good idea if the auto-commits are limited to formatting/hygiene fixes only.

If you want to open a PR, I’ll do some tests with it.

[prateek-dagar]

Hi @andersonhc,

I have raised PR #1918 for this.
Please have a look and let me know if any changes are required!