adapt Pre commit hook and black

[RonnyPfannschmidt]

prepare #3464

[blueyed]

Where/how is this config used?

[RonnyPfannschmidt]

@blueyed by the pre-commit tool - its generalized pre commit hooks a dev can opt into

[coveralls]

Coverage increased (+0.05%) to 92.744% when pulling 437a6fb on RonnyPfannschmidt:pre-commit-hook into 65bc43d on pytest-dev:master.

[blueyed]

Ah.. for reference: https://pre-commit.com/

[nicoddemus]

Looking good, but we also need to update CONTRIBUTING.rst on how to install and use the hooks.

[nicoddemus]

The CI failures seem related to #3480. Merging master into features in #3490, after that is merged we can try to rebase this.

[nicoddemus]

I looked at the docs, thanks, looks good!

I tried your branch locally and would like to comment on a few points:

1. It seems black did not run on _testing: when I run black testing I get changes in 24 files (see diff).

2. We should add a black --check to the linting environment as well, people can forget to install pre-commit or even by pass the pre-commit checks with git commit -n. I was going to merge this PR and open another one containing this, but because of the first point I was left wondering if I'm losing something.

3. I think we should squash all commits under a "Apply black formatting and pre-commit hooks" umbrella.

[RonnyPfannschmidt]

@nicoddemus this is consistent with the pinned black versions - else we get failing pre-commit on black upgradeds - i wil upgrade to the latest stable version however

having black in the linting env creates a double failure point as its in need of version syncing

i pan to set up a travis stage for the linting checks however

[RonnyPfannschmidt]

@nicoddemus after reiterating and going back to the older black version due to flake8 errors being introduced i reformatted a missing file and would like to get it merged including history for later comprehension

[nicoddemus]

Oh I see, different black versions being used, it makes sense thanks.

For linting we should use pre-commit run to avoid duplicating the checks in tox.

I think it is important to add the linting stage before merging to avoid getting commits that don't pass the checks, don't you agree? It should be simple to add a new travis stage for that.

[RonnyPfannschmidt]

in that case we will have to defer and possibly rebase this one due to investigating sane caching for pre-commit on travis

[nicoddemus]

in that case we will have to defer and possibly rebase this one due to investigating sane caching for pre-commit on travis

Why, do you think it will take a long time to install the pre-commit hooks? It takes less than a minute in my computer the first time, I don't think that's something to worry about.

[nicoddemus]

Nice! It seems to have worked wonderfully!

Please update our linting tox env to run pre-commit instead and run tox -e linting in the linting stage for completeness. 👍

After that I think we are in excellent shape, excellent work!

[RonnyPfannschmidt]

i'd prefer to jsut drop the linting then - i'll take a look at getting ti to be pre-commit only as well

@obestwalter it would be nice to have tox plugin for certain kinds of envs there

[nicoddemus]

i'd prefer to jsut drop the linting then - i'll take a look at getting ti to be pre-commit only as well

You are right, dropping it is fine. If you do, please update CONTRIBUTING because we mention linting in there.

A small note: I just noticed that the linting stage has TOXENV=coveralls; I know it doesn't matter but it would be nice to get rid of that as it might be confusing.

[RonnyPfannschmidt]

@nicoddemus i just moved it to use pre-commit

[nicoddemus]

Thanks! Shouldn't the linting stage just run tox -e linting for consistency then?

(Sorry for being so pedantic, just wanting to make sure we are not leaving loose ends).

[obestwalter]

Hi @RonnyPfannschmidt, best might be to open a "plugin request issue" in tox repo (would be the first, I guess) explaining what you imagine.

[RonnyPfannschmidt]

@nicoddemus i want the linting stage to be fast, whats missing now is a cache for pre-commit

[nicoddemus]

@nicoddemus i want the linting stage to be fast, whats missing now is a cache for pre-commit

It went from 50s to 2m30s. While this is a significant increase, it is not really relevant considering that the full build takes >30m.

Just saying I'm not particularly concerned with it, but if you really want to improve that timing then go for it. 👍

[RonnyPfannschmidt]

@nicoddemus i'd also like to put pypy into a own stage in some way

[nicoddemus]

Just so we keep track, I think the only thing missing is this:

Shouldn't the linting stage just run tox -e linting for consistency then?

---

@nicoddemus i'd also like to put pypy into a own stage in some way

Not sure why, but feel free to do this in another PR.

[nicoddemus]

@RonnyPfannschmidt gentle ping in case you didn't see my last comment. 👍

[obestwalter]

You might want to add the code style badge to the README also?

.. image:: https://img.shields.io/badge/code%20style-black-000000.svg
  :target: https://github.com/ambv/black
  :alt: Code style: black

[RonnyPfannschmidt]

@nicoddemus i dont want to use tox for the linting since its an extra nesting i dont want to have

[nicoddemus]

OK, I fixed the two conflicts, if I have done everything correctly we should be able to merge it after all is green.

[nicoddemus]

Formatting the merged files and will push in a minute.

[nicoddemus]

🎉