Wow, this seems to be moving fast. Maybe the deadline is a little bit tight given that we haven't fully figured out what we want and haven't even tested it? Here are some of my considerations:

• The suggested hooks look good, although it seems we could also enable a bunch of further hooks that come built-in with pre-commit (e.g. check-ast, check-case-conflict, end-of-file-fixer, mixed-line-ending, trailing-whitespace). I don't think we need any auto-fixers like autoflake8, esp. not in pre-commit hooks. Plus mypy and flake8 are at times already hard enough to reign in, so I guess we're already good in terms of code analysis.
• We still need to agree on a doc comment style and tooling for supporting that in Docstring formatting #344
• We are already running flake8 and mypy via tox and also on every pushed commit in GitHub Actions. Which checks do we want to forcibly run locally on every commit, which ones do we want to run online in GitHub Actions / on-demand offline via tox? In my opinion, commit hooks should be sufficiently fast to not interrupt your workflow esp. when doing multiple small commits. So we should find a good split here -- also given that running the "optional" checks locally is just as easy (if you remember to do it) thanks to tox. Maybe the pre-commit hooks should just check formatting and the more heavy-weight things should be called directly?
• Also, there are few configuration options we might wanna think about. Having a screen that is more wide than tall, I usually prefer lines longer than 80 characters over a pyramid of indentations built before a line broken into five overly indented overly short parts. So, I'd prefer to have a line length greater than 100, and anyways this configuration probably needs synchronization across tools.
• How should we proceed here? My suggestion would be - in parallel while trying to get as many PRs as possible merged - to already set up a PR where our selection of tools ran and where we can inspect the results. Once we agree that we are good to go, we can always re-run all the checks on the current master and then force-push and merge said PR.

Wow, this seems to be moving fast. Maybe the deadline is a little bit tight given that we haven't fully figured out what we want and haven't even tested it? Here are some of my considerations:

• The suggested hooks look good, although it seems we could also enable a bunch of further hooks that come built-in with pre-commit (e.g. check-ast, check-case-conflict, end-of-file-fixer, mixed-line-ending, trailing-whitespace).

A few of these seem to be covered already e.g. by black

I don't think we need any auto-fixers like autoflake8, esp. not in pre-commit hooks. Plus mypy and flake8 are at times already hard enough to reign in, so I guess we're already good in terms of code analysis.

• We still need to agree on a doc comment style and tooling for supporting that in Code formatting #344

Seems like this can be addressed in a follow up PR.

• We are already running flake8 and mypy via tox and also on every pushed commit in GitHub Actions. Which checks do we want to forcibly run locally on every commit, which ones do we want to run online in GitHub Actions / on-demand offline via tox? In my opinion, commit hooks should be sufficiently fast to not interrupt your workflow esp. when doing multiple small commits. So we should find a good split here -- also given that running the "optional" checks locally is just as easy (if you remember to do it) thanks to tox. Maybe the pre-commit hooks should just check formatting and the more heavy-weight things should be called directly?

Are there specific checks in this list that have performance concerns?

"if you remember to do it" is the issue -- keep in mind the time spent if you miss a check, and find out later in a github action. It's much less hostile to just enforce with a pre-commit hook. This seems like an established best practice, and not really an issue in practice given the speed of most of these invoked from pre-commit.

• Also, there are few configuration options we might wanna think about. Having a screen that is more wide than tall, I usually prefer lines longer than 80 characters over a pyramid of indentations built before a line broken into five overly indented overly short parts. So, I'd prefer to have a line length greater than 100, and anyways this configuration probably needs synchronization across tools.
• How should we proceed here? My suggestion would be - in parallel while trying to get as many PRs as possible merged - to already set up a PR where our selection of tools ran and where we can inspect the results. Once we agree that we are good to go, we can always re-run all the checks on the current master and then force-push and merge said PR.

Wow, this seems to be moving fast. Maybe the deadline is a little bit tight [..]

Indeed, i'm sometimes a bit impatient 🙈 Also, the target was more to merge the others PR so we are clear to make big changes, not to merge the black PR today.

it seems we could also enable a bunch of further hooks that come built-in with pre-commit

A few of these seem to be covered already e.g. by black

It could still be nice to enable check-case-conflict, end-of-file-fixer, mixed-line-ending, trailing-whitespace to fix the non-python files imo.

Are there specific checks in this list that have performance concerns?

To me, mypy is a bit too slow to be enabled in the pre-commit , i think. (But i don't have a very strong opinion on this.)

So, I'd prefer to have a line length greater than 100

I'm also for longer line lengths in general, the time where we had very small screens is long behind us. (But again, not a strong opinion)
EDIT: black already uses 88 and not 80. They seem to have given some thought in it and not using 100 seems a deliberate, thoughtful decision so i'd be keen on keeping it (but again, i don't really care).

My suggestion would be [..] set up a PR where our selection of tools ran and where we can inspect the results.

Here it is: main...formatting-results

Nice, thanks for running all these things! Do you recall how long the individual steps / the overall process took? The results look pretty good to me, especially because it seems that the limit is not strictly enforced for strings, where breaking up would really hinder readability. The only weird thing is how format strings are sometimes replaced by the "{}".format() notations, sometimes converted to f-strings and sometimes left as-is. As f-string were introduced in 3.6, they would be the way to go everywhere - do we have a way about warning when the other formats are used?

After the first run (that installs the tools, etc), a full run takes 2.57s on my machine. But we also have to keep in mind that a full run is almost never done as by default, pre-commit runs only on modified files so it's much much faster.

$ time pre-commit run --all
fix end of files.........................................................Passed
trim trailing whitespace.................................................Passed
check for case conflicts.................................................Passed
mixed line ending........................................................Passed
pyupgrade................................................................Passed
isort....................................................................Passed
black....................................................................Passed
blacken-docs.............................................................Passed
pre-commit run --all  2.57s user 0.66s system 323% cpu 1.000 total

format strings are sometimes replaced by the "{}".format() notations, sometimes converted to f-strings and sometimes left as-is

This is weird, i'll have a look at the options of pyupgrade to try and force f-strings everywhere.

The doc of pyupgrade says :

pyupgrade is intentionally timid and will not create an f-string if it would make the expression longer or if the substitution parameters are sufficiently complicated (as this can decrease readability).

And i see no option to override this behaviour.

I had a look around the internet and it seems that there is another tool to do this: flynt.
Here is an example of what it could output if we also add is as a pre-commit: e9735cc

Note: what we could also do, is to run it once right now to get rid of the legacy and then rely on pyupgrade afterwards, this might be good enough.

Note: what we could also do, is to run it once right now to get rid of the legacy and then rely on pyupgrade afterwards, this might be good enough.

I've done that in several projects (e.g., https://github.com/networkx/networkx) and it worked well.

It took a few commits to pass cleanly the tests but i'm happy to say that it's now merged and that we have a GitHub action enforcing the pre-commits !