Add pre-commit configuration and instructions

[stefanv]

I recommend we switch to pre-commit and disable pep8speaks. Turns out, if you don't enforce it it gets ignored :) I think this is actually a friendlier experience for contributors, since they get to sort out all the annoying PEP8 nitpicks before review.

Note, the test suite will now definitely fail, but I didn't want to pollute this PR with thousands of PEP8 tweaks.

We can also adjust the PEP8 rules we want to enforce, although I already disabled the ones I think we don't care about.

[hmaarrfk]

I would really prefer if we tabled the instruction update to avoid a conflict.

Running pep8 on all builds is also annoying since linting disrupts general tests.
A dedicated build for linting would be appreciated.

[stefanv]

The whole point of the pre-commit hook is that this test should never fail. When it does, rather fail early? A special entry just for linting feels like overkill (but, if that's general consensus, ok).

Why would the instruction update generate a conflict? And, if it does, would it be hard to fix?

[grlee77]

Is this mypy stuff needed or is it just here for future use? We didn't actually commit any typed code yet did we?

[grlee77]

I have used a flake8 pre-commit on some other projects and have liked it so far.

[emmanuelle]

My two cents: the pep8-quality of our code is reasonable, and so is the quality of code submitted by contributors (and I think pep8speaks is useful, most people). I err on the side of less infrastructure imposed on contributors. pep8 is here so that code is nice and fast to read because of consistency with other projects, I think our code is nice enough to read.

[stefanv]

@emmanuelle I don't quite understand: this PR only enforces pep8, and makes it easier for developers to catch those errors early? I don't think it imposes more than pep8speaks. It just makes sure people notice it, since pep8speaks clearly often is ignored (witness how many our files are not pep8 compliant).

[emmanuelle]

@stefanv sorry if I was not clear. What I meant basically is that it's one more step (pip installing pre-commit) to do before one can submit a PR.

[hmaarrfk]

I err on the side of less infrastructure imposed on contributors.

I agree with Emanuelle's sentiment.

[grlee77]

I find the pre-commit approach more convenient than finding out after creating the PR that I forgot to fix PEP-8 issues. pep8speaks does work fine, though, so I don't have a real problem sticking with that approach either.

It is one additional thing to set up as a first-time contributor, but it seems fairly easy relative to learning to use git itself.

[jni]

The whole point of the pre-commit hook is that this test should never fail. When it does, rather fail early? A special entry just for linting feels like overkill (but, if that's general consensus, ok).

An important point is that if a contributor has not installed the pre-commit hook (this has happened even to me on the napari repo when I was working on a new machine, for example), it's useful information for maintainers to know whether the actual code is working even if the linter fails. So, for napari we have separate checks for both linting and formatting and that's been working well.

[jni]

pep8speaks clearly often is ignored (witness how many our files are not pep8 compliant).

But very often it should be ignored, e.g. in array literal formatting... All of these auto-formatters ignore the opening clause of PEP8.

[stefanv]

It becomes very easy to run this as part of the CI by simply invoking pre-commit the same way the user would.