pre-commit/black is getting in the way too much

[blueyed]

It is annoying to get interrupted with something like:

% gc
[WARNING] Unstaged files detected.
[INFO] Stashing unstaged files to /home/user/.cache/pre-commit/patch1583628686.
black....................................................................Failed
- hook id: black
- files were modified by this hook
[20+ lines deleted]
[INFO] Restored changes from /home/user/.cache/pre-commit/patch1583628686.

to see that black did turn it into something less readable (since it does
not recognize "exploding commas" with function calls):

-    result.stdout.fnmatch_lines([
-        'E       assert False',
-        '*- Captured stdout call -*',
-        'hello',
-    ])
+    result.stdout.fnmatch_lines(
+        ["E       assert False", "*- Captured stdout call -*", "hello",]
+    )

Therefore I think it would be more sensible to either exclude black from the
pre-commit config altogether, or not to recommend installing pre-commit in the first place.

I think it is fine to enforce checks on CI, but that could/should be limited to
flake8 etc only, and black could be run then in the release process.

Black is meant to have consistent output for things being committed, but it does not help if you have to add # fmt: {on,off} (after being interrupted), or just accept that it is less readable etc.
I.e. in the second case it certainly causes code to be less readable / and more affected to diff churn (when tests get hardened/changed with regard to their output).

[hugovk]

I think this magic comma will be fixed in psf/black#1288, but you could ask there to double check.

[RonnyPfannschmidt]

i consider black a integral part of keeping certain review overheads low,
i'm not going to agree with removing it

[blueyed]

@hugovk the issue is older already (psf/black#1010).
And even if it was fixed, it might still get in the way (with new issues etc).

(FWIW I agree with using black in general, and often use it manually. I've disabled pre-commit myself for now (making git-commit snappy again), and will see how that goes - since there is a distinct "lint" job on CI where black gets run, it's easy to spot/fix anything missed then. And yes, I am aware of --no-verify already)

[nicoddemus]

Definitely 👎 on removing black.

[blueyed]

@nicoddemus sorry, you've missed the point (again) - this was not about removing black, but only not making it mandatory via pre-commit etc. See the refs above.

[nicoddemus]

Sure, sorry, but either way you are definitely free to uninstall pre-commit and run tox -e linting manually, but we should not recommend that in general, as people often get it wrong.