Workflow: recommend pre-commit for code checks

[TomAugspurger]

pre-commit is a library for running pre-commit hooks (actually does more that just pre-commit). Adding additional code checks is likely to annoy first-time contributors (isort); we can use tools to avoid that.

I've been using this .pre-commit-config.yaml

repos:
    - repo: https://github.com/pre-commit/pre-commit-hooks
      rev: 2dbaced6501ea775d9e5a7677c49627899a17756
      hooks:
          - id: flake8
            language: python_venv
    - repo: https://github.com/pre-commit/mirrors-isort
      rev: f35773e46d096de5c45365f1a47eeeef36fc83ed
      hooks:
          - id: isort
            language: python_venv

With that in the root of my directory, every time I git commit, those code checks are run.

Suppose I have an out-of-order import, and go to commit my changes

bash-4.4$ git add .
bash-4.4$ git commit -m 'fixup'
Flake8...................................................................Passed
isort....................................................................Failed
hookid: isort

Files were modified by this hook. Additional output:

Fixing /Users/taugspurger/sandbox/pandas/pandas/core/arrays/array_.py

The hook modified the file in place. I can check the status / diff from HEAD

bash-4.4$ git status
On branch pd.array
Your branch is up to date with 'origin/pd.array'.

Changes to be committed:
  (use "git reset HEAD <file>..." to unstage)

        modified:   pandas/core/arrays/array_.py

Changes not staged for commit:
  (use "git add <file>..." to update what will be committed)
  (use "git checkout -- <file>..." to discard changes in working directory)

        modified:   pandas/core/arrays/array_.py

add them, and commit again

bash-4.4$ git add .
bash-4.4$ git commit -m 'fixup'
Flake8...................................................................Passed
isort....................................................................Passed
[pd.array 5fcf609f3] fixup
 1 file changed, 4 insertions(+)

before pushing.

---

What are people's thoughts. I think we can distribute a .pre-commit-config.yaml with the git repo. If pre-commit isn't installed, I assume nothing will happen.

[datapythonista]

+1 on this, but I'd just add things that are immediate or almost immediate to be checked (e.g. git diff upstream/master -u -- "*.py" | flake8 --diff, which wouldn't be necessary in the PR template).

It'll be some work to check things mainly based on the diffs so they are fast (I'm thinking for example in docstring changes), but if we manage to do it, I think it should make our life, and contributors life much easier.

[TomAugspurger]

I don’t know exactly what flake8 is doing, but it’s been fast in my experience. Probably just flake8ing the files that changed. You can also skip checks with the right flags.

…

________________________________ From: Marc Garcia <notifications@github.com> Sent: Saturday, November 10, 2018 6:35:09 PM To: pandas-dev/pandas Cc: Tom Augspurger; Author Subject: Re: [pandas-dev/pandas] Workflow: recommend pre-commit for code checks (#23616) +1 on this, but I'd just add things that are immediate or almost immediate to be checked (e.g. git diff upstream/master -u -- "*.py" | flake8 --diff, which wouldn't be necessary in the PR template). It'll be some work to check things mainly based on the diffs so they are fast (I'm thinking for example in docstring changes), but if we manage to do it, I think it should make our life, and contributors life much easier. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub<#23616 (comment)>, or mute the thread<https://github.com/notifications/unsubscribe-auth/ABQHIl8gNU_ReAUCXeJ-Yn7HNjx4Inqeks5ut3C9gaJpZM4YYK1m>.

[jreback]

btw i find that putting things in the Makefile is nice for interactive development

eg make lint-diff

[gfyoung]

+1 from me as well. Good sanity checks.

[h-vetinari]

@TomAugspurger in #23707:

I'd be curious to hear your thoughts on #23616.
If we recommend using pre-commit hooks (after verifying that hey work on all platforms) we could remove this doc section entirely.

I'm quite strongly against precommit hooks (esp. modifying content in place). I tried them myself recently to deal with ipython notebooks, but the problem is that, generally speaking:

• there's a difference between committing and pushing (not every commit needs the equivalent of a lint check)
• modifying inplace is bad, and not just in situations where commits are aborted (empty commit), etc.
• git can't enforce people using the hooks (well, aside indirectly through the serverside CI itself), so you can't rely on that (e.g. removing the box for the code checks from the PR template). Aside from the fact that installing githooks is another barrier to entry for new developers.

I would say that (was gonna comment the same in #23658):

• with the recent changes to the CI, git diff upstream/master -u -- "*.py" | flake8 --diff doesn't cut it anymore.
• it should be replaced with a single script (ideally platform-agnostic, at least from a user-POV) that tests all the relevant lint checks (flake8, isort, doctests), which should IMO have a box in the PR template. (this script could be eventually enhanced with all sorts of things; e.g. check that dependencies are up to date; run tests that were changed by PR, etc.). DOC: flake8-per-pr for windows users #23707 is maybe a small step in that direction.
• (optionally), one could consider recommending to add that to the git pre-push-hook, rather than the pre-commit-hook.
• it should IMO still be possible to push a failing commit (maybe with a prompt somehow) - e.g. I'm on a platform where pandas has some failures in the current test suite.

[TomAugspurger]

there's a difference between committing and pushing (not every commit needs the equivalent of a lint check)

Hopefully these checks are fast enough that they don't slow you down. I haven't found them to be troublesome.

modifying inplace is *bad*, and not just in situations where commits are aborted (empty commit), etc

The isort hook does modify files in place, but does not stage the changes. I've found the workflow of ``` git add . git commit -m ... # isort failure git diff # inspect what isort did git add . git commit -m ... ``` to be quite nice.

git can't enforce people using the hooks

We can't enforce that contributors follow the issue template either :) We'll still have the CI to enforce things. This is just to ease the annoying push, wait for CI, see small linting mistake cycle.

Aside from the fact that installing githooks is another barrier to entry for new developers.

I found pre-commit to be pretty painless. Have you used it? pre-commit does support pre-push hooks. If we like this, we can start adding some more expensive checks as pre-push checks to the config we distribute.

[jorisvandenbossche]

@TomAugspurger this can be closed now after #27233 ?