Do you use 'make patchcheck'?

Core Development
1

We’re currently moving the checks in Tools/patcheck to run via pre-commit in GH-109408 and related PRs.

In discussion, it has transpired that running make patchcheck (or python.bat Tools\patcheck\patcheck.py) locally performs several checks to remind contributors of what goes into a good PR, see the devguide for an overview of what is reported upon.

However, we don’t know how commonly patchcheck is used today, and if it makes sense to keep the tool, keep the checks but within a different tool, remove both, or something completely different.

To guage this, please may you indicate your typical behaviour in the following polls? Written notes are of course also welcome.

Using make patchcheck when preparing a PR
  • I typically use make patchcheck or python.bat Tools\patcheck\patcheck.py
  • I do not use make patchcheck or python.bat Tools\patcheck\patcheck.py
0 voters
Utility of the patchcheck checks
  • I think the checks that patchcheck uses are useful for contributors, including newer contributors
  • I do not think the checks that patchcheck uses are useful for contributors, including newer contributors
0 voters
A make target for linting
  • I would use a make lint target to run all the linting checks that CPython provides
  • I wouldn’t use a make lint target, or I do not think such a target should be in the Makefile
  • I do not use make (e.g. for Windows users)
0 voters

A

3 Likes
2

For the second question, I voted in affirmatively that “I think the checks that patchcheck uses are useful for contributors, including newer contributors”, but I think we should move the checks to pre-commit or similar, and these can be wrapped in a make command for ease of use.

As an extension for the third question, we can adapt make patchcheck to run all the lint checks: so it does all the patchcheck.py ones, plus the extra pre-commit checks. As we move checks from patchcheck.py to pre-commit, it would still run both.

And we could have make lint do the same as make patchcheck. Or make lint could just be the pre-commit things.

4 Likes
3

Thanks to Hugo for the (implicit) prompt, I realise I might not have explained this very well. The idea behind question 2 is to divorce the checks that patchcheck performs from the tool itself – if there’s consensus that the checks are useful but patchcheck is not, we could try and move them to some other venue, as Hugo alludes to.

A

1 Like
4

I voted no on that question, because a number of the checks seem to be situational (has the documentation/test suite been updated, has misc/ACKS been updated). An automated check that can be ignored if you think it doesn’t apply tends to teach people to just ignore the check altogether, in my experience. So I think it’s a good checklist, but I’m not sure that translates well to them being good checks.

But I will admit I’m now confused because I don’t understand what “divorcing the checks from the tool” implies. If it includes “having a checklist in the devguide is useful and sufficient”, then I’m fine with that. If it means running some automated process and reporting “failures”, then I’m not so happy with it.

5

My “I don’t use it” and “I think the checks are useful” vote may seem contradictory, but it reflects my workflow: I rely heavily on CI - which is more natural now that Draft PRs are a thing so I don’t feel like I’m spamming people with unfinished work.

What I could potentially imagine as nicer than make patchcheck or make lint is actually make push… that runs meaningful checks that must pass prior to doing a git push of my branch to a git repo name of my choosing.

3 Likes
6

I think there is a discoverability issue. The last time I prepared a PR, I wanted to run those checks but I didn’t remember the spelling. There used to be an automatic reminder (was it back when using hg?) asking the user if they had run make patchcheck, which I think was quite useful.

I don’t know how git pre-commit hooks work, but if they are typically blocking, I would hate for a git commit to stall for seconds until a non-trivial command finishes running. Also, there are reasons to push a commit that does not validate all checks (for example to exercise some potential solution against CI).

3 Likes
7

If CI (or anything else) alerts me that a lint failed, I’d welcome a way to reproduce that failure locally (like make lint), so I don’t need to upload all the iterations of the fix.

Ideally, the mechanism wouldn’t automatically install blobs from PyPI onto core devs’ computers.

That could be git push: like the pre-commit hook (a local script that Git runs before you commit), there’s a pre-push hook that runs before pushing.

3 Likes
8

No problem, the pre-commit tool can be run in two ways.

  1. You can install it as an actual pre-commit hook, so it runs when you git commit, and will fail the commit if the checks fail. I have this enabled and really like it, but understand it’s not for everyone, so it definitely won’t be required for anyone.

  2. The other way is to run it as a command-line tool, and run it on demand. This is what the CI does, and we can have a make command abstract away the flags and things to remember. It won’t run when you commit, and won’t slow down commits. You can then push and the let the CI do its thing.

(Aside: with pre-commit installed as a hook, you can git commit -n / --no-verify for it to skip the checks.)

4 Likes
9

A git integrated pre-hook like this would be ideal for me… because honestly i’m not in the habit of running make for anything other than the main build. (and will happily run whatever other tool for that step should our build system ever change)

10

I don’t use patchcheck because IDLE has the equivalent for the file being edited and because I check diffs in git gui before committing and the latter marks trailing whitespace in a distinct color.

2 Likes