Revamp linting processes

[pradyunsg]

Annoyance driven development! \o/

I got annoyed enough at our current processes for linting that, well, I've done work to switch us over to using pre-commit for linting stuff.

Details on pre-commit can be found on https://pre-commit.com.

Why switch to using pre-commit?

pre-commit is a fairly powerful framework for, well, "identifying simple issues before submission to code review". It's basically a framework for running/building linters. Our linters are still the same -- isort, flake8 and mypy. Using pre-commit simplifies tracking/updating which versions we're using and how we're invoking them.

We have a single configuration file describing which linter we're using, what version of the linter and any additional details for how to invoke them. pre-commit hooks are nice/smart enough to automatically ignore/filter out things that we shouldn't be linting anyway. Linter versions can also be automatically updated by running pre-commit autoupgrade, which bumps the "pins" to the newest version. The next invocation of tox -e lint would use the newer pins.

Functionally, all this means less duplication of "don't lint this", less number of environments to directly create/manage within our CI (pre-commit creates the environments on its own), nicer output, more comprehensive checks and unlocking us to more capable checks (add-trailing-comma works best when used with pre-commit: asottile/add-trailing-comma#59 😉). None of our commands for invoking the linters have changed.

Perhaps most importantly, I think this is a nicer development experience and there are less CI jobs.

How does this affect the development workflow?

The main (only?) development-flow-affecting change in this PR, is tox -e lint would do everything (and a bit more than) that tox -e lint,mypy,packaging do today.

There's no longer tox -e mypy/tox -e packaging -- they've been bundled into tox -e lint. The same is true for nox -s validate_news. tox -e lint-py2 has been dropped.

What's in this PR

Each commit is atomic (at the time of filing this PR), so I hope you can tell what exact changes were made looking at the commit message summaries that GitHub shows. :)

The best way to review this PR is going to be, to review commit-by-commit.

---

Every "Enable ..." commit enables a single pre-commit hook (a hook = a linter/check) and makes the changes that necessary to pass those checks. Note that some of these hooks are capable of automatically fixing the code.

[pfmoore]

Can you clarify how this would work for a development workflow, please?

I've never used pre-commit, and it seems to me that it's fairly tightly coupled to your development environment, which could be a pain point for me. To clarify, my development environment is typically pretty much non-existent. When working, I tend to create a fresh git clone of my pip fork, do the usual git pull upstream master; git push dance to get up to date, and then simply start editing in vim. The test suite is so slow on Windows that I generally don't even try to run it locally in the initial stages of a change, I just push to my fork and let CI handle it. I catch many of my typos this way, too :-( If I do decide to lint my code locally (usually after 2 or 3 rounds of CI remind me that my typing is dreadful!) I just install tox in a new, temporary venv (pew mktmpenv) and run tox with the relevant lint target. I pretty much never go near isort, just manually fixing ordering errors that get flagged.

Would a similar sort of workflow still work with this change, or would I need to install pre-commit in my temporary venv? Would I need to add git configuration to enable the pre-commit hooks to work?

I do think it's important that we optimise the development process for "minimal setup" users. Not just to suite me (🙂) but also because it encourages casual contributors.

[pradyunsg]

(ah heck. I accidentally closed the tab with my response to you)

[pradyunsg]

(this is a much shorter response, that's less fun too)

Can you clarify how this would work for a development workflow, please?

Sure, more than happy to. :)

Would a similar sort of workflow still work with this change,

Yes.

The main (only?) development-flow-affecting change in this PR, is tox -e lint would do everything (and a bit more than) that tox -e lint,mypy,packaging do today. There's no longer tox -e mypy/tox -e packaging -- they've been bundled into tox -e lint.

would I need to install pre-commit in my temporary venv?

No.

tox -e lint still works. I'll keep using that myself.

It'll create an environment with pre-commit and invoke it. That's already needed for cooperating with our CI, so you'd be doing exactly what our CI is doing.

That said, directly using pre-commit in your temporary environment would likely be a better experience (using pre-commit's CLI would be more flexible than the only-one-way invocation in tox) but it's not needed by any means, for the "minimal setup" reasons you pointed to.

Would I need to add git configuration to enable the pre-commit hooks to work?

No.

I do think it's important that we optimise the development process for "minimal setup" users.

Strongly agree.

If you'll look at "Getting Started" in this PR, you'll notice that I haven't made any changes to what's needed for pip's development. That's because it hasn't changed.

What's changed is that instead of having all linters/tools getting invoked individually within tox or whatever, we're using pre-commit to make invoking them (and their outputs) easier to deal with.

---

The following is the output of tox -e lint, when all the checks we have are passing:

(ignore the bad colors, that's because my CLI configuration is a mess)

[pfmoore]

The main (only?) development-flow-affecting change in this PR, is tox -e lint would do everything (and a bit more than) what tox -e lint,mypy,packaging does today.

Excellent, thanks for clarifying. I'd looked at pre-commit a couple of times before and got intimidated because the docs seemed to assume that I had a far more organised development environment than I actually do :-) So sorry if I seemed a bit paranoid as a result.

If you'll look at "Getting Started" in this PR, you'll notice that I haven't made any changes to what's needed for pip's development. That's because it hasn't changed.

Cool. I hadn't looked into the detail of the PR, I'm very low on time these days due to home life stuff, so I'm skimming things even more than I usually do. Thanks for bearing with me.

In principle I like this - anything that tidies up our processes is great. I'll try to do a more thorough review, but please don't wait for me if I don't get to it.

[atugushev]

@pradyunsg waiting eagerly for black :)

[pradyunsg]

@atugushev FYI -- #5425

[pradyunsg]

Okay, I'm happy with where this PR is now.

The only concern I have is related to twine check -- which does README rendering checks -- is only running on Azure Pipelines right now. I don't think it's a major issue, so I'd say this is fine and we can improve it later.

[atugushev]

@pradyunsg

FYI -- #5425

It will happen someday (i hope), for pre-commit+black is a killer feature now.

[pradyunsg]

I'll try to do a more thorough review, but please don't wait for me if I don't get to it.

Awesome! Thanks @pfmoore! ^>^

---

Thanks for reviewing this @chrahunt! ^>^

[chrahunt]

LGTM.

[hugovk]

👍

I used to dislike pre-commit in general, but I'm finding it useful now and pre-commit makes it easy.

It's a good way to run some quick linting things before pushing, to find problems right now and avoid waiting for the CI to find them.

When you have more than one lint thing, pre-commit can combine them into a single command, good for a CI and/or tox, regardless of using it locally for commits.

If you don't like pre-commit, you don't need to use it locally, just commit as normal and let the CI catch things.

If you do have it installed locally, but want to skip it for a given commit, use -n, --no-verify:

git commit -m "message" -n

[pfmoore]

LGTM. A few minor comments.

[pradyunsg]

Given the two approvals and positive feedback/attitude from contributors toward doing this, I'm gonna go ahead and merge this PR.

[pradyunsg]

Hurrah! I'm happppy that this actually got done finally. :)

To be abundantly clear, I do think we should iterate on improving these linters and checks, now that we have started using a nice framework for them.

This PR included the most basic ones that I think we should definitely have and that have feature-parity with our current checks. I think we should look into stuff that we might want to have (like add-trailing-commas) that are actually an improvement over what was the status quo for us.

Thanks for taking the time to review this @chrahunt and @pfmoore; and chiming in @atugushev and @hugovk! Much appreciated.

[pradyunsg]

/cc @RonnyPfannschmidt since he'd suggested this once in a different PR somewhere. (my notes say that)