gh-109408: Add the docs whitespace check from patchcheck to pre-commit by hugovk · Pull Request #109854 · python/cpython · GitHub

Moves one of the patchcheck checks to pre-commit.

1.

patchcheck's normalize_docs_whitespace runs on files where fn.startswith('Doc') and fn.endswith(('.rst', '.inc')

pre-commit's trailing-whitespace is already running on c, python and rst in the whole codebase.

So this widens the check to cover inc files in the whole codebase, not just in Doc. I think this is a good thing.

2.

patchcheck substitutes matches of re.compile(br'\s+(\r?\n)$') with br'\1. That is, it strips multiple trailing \r and \n characters.

pre-commit's trailing-whitespace trims all trailing whitespace by default (docs). This also widens the check, I think this is a good thing.

3.

Remove docs_modified(doc_files) from patchcheck, it merely printed out whether any docs files were modified. I don't think it's useful now patcheck does no docs checking, and I'd like to eventually remove patchcheck.

4.

Some people might be still running make patchcheck? If so, to maintain some kind of parity, I've added steps to install and run pre-commit as part of the Makefile command.

We're doing a similar thing in the PEPs repo:

https://github.com/python/peps/blob/a159a9ac58ca73dc1da0b626b6df77807af455d0/Makefile#L62-L77

(And Pillow: https://github.com/python-pillow/Pillow/blob/main/Makefile)

Does this seem useful, or better to leave it out?

5.

Finally, some cleanup of patchcheck.py: whitespace, f-strings, consistent pluralised outputs, fix typo.

Issue: Move Azure Pipelines CI to GitHub Actions #109408

…-commit's trailing-whitespace

Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com>

LGTM (though I can't review the Makefile changes :)

I don't think make patchcheck should run pre commit, especially if the goal is to remove it. Hence I'd say this is a docs point to communicate.

Suggested change

      
            	@$(RUNSHARED) ./$(BUILDPYTHON) -m pre_commit --version >/dev/null 2>&1 || $(RUNSHARED) ./$(BUILDPYTHON) -m pip install pre-commit >/dev/null 2>&1
          
            	$(RUNSHARED) ./$(BUILDPYTHON) -m pre_commit run --all-files

I'm thinking it would be a good idea to add a make lint target like we have for the PEPs repo (and Pillow), which seems to work there.

Perhaps we should add make lint now and have it run pre-commit; and maybe also patchcheck?

I'd agree. We should also update the devguide (currently only patchcheck is mentioned)

I've removed the Makefile changes from this PR, let's decide what to later after discussion:

https://discuss.python.org/t/do-you-use-make-patchcheck/34743?u=hugovk

Co-authored-by: Adam Turner <9087854+AA-Turner@users.noreply.github.com>

Right now Makefile does not have pip mentions at all, I think that adding a new target with external deps needs a bit of discussion.
Since Makefile has a biiig range of different use-cases.

In my opinion having pre-commit CLI and in CI on its own is good enough.

Co-authored-by: Adam Turner <9087854+AA-Turner@users.noreply.github.com>

Perhaps we should add make lint
In my opinion having pre-commit CLI and in CI on its own is good enough.

To clarify my opinion: I don't mind which of either of these are used, as I use Windows so the choice is personally irrelevant! I'll be happy with whatever Hugo goes for.

A

We could perhaps create a poll on Discourse in the core development category asking how many people actually run make patchcheck. If the answer is zero (or very few), then that simplifies the decisions that need to be here — we can just get rid of it from the Makefile

Written up some questions at https://discuss.python.org/t/do-you-use-make-patchcheck/34743

A

Thanks @hugovk for the PR 🌮🎉.. I'm working now to backport this PR to: 3.11, 3.12.
🐍🍒⛏🤖

…-commit (pythonGH-109854) (cherry picked from commit 7426ed0) Co-authored-by: Hugo van Kemenade <hugovk@users.noreply.github.com> Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com> Co-authored-by: Adam Turner <9087854+AA-Turner@users.noreply.github.com>

GH-110594 is a backport of this pull request to the 3.12 branch.

GH-110595 is a backport of this pull request to the 3.11 branch.

…e-commit (GH-109854) (#110595) gh-109408: Add the docs whitespace check from patchcheck to pre-commit (GH-109854) (cherry picked from commit 7426ed0) Co-authored-by: Hugo van Kemenade <hugovk@users.noreply.github.com> Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com> Co-authored-by: Adam Turner <9087854+AA-Turner@users.noreply.github.com>

…e-commit (GH-109854) (#110594) gh-109408: Add the docs whitespace check from patchcheck to pre-commit (GH-109854) (cherry picked from commit 7426ed0) Co-authored-by: Hugo van Kemenade <hugovk@users.noreply.github.com> Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com> Co-authored-by: Adam Turner <9087854+AA-Turner@users.noreply.github.com>

…-commit (python#109854) Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com> Co-authored-by: Adam Turner <9087854+AA-Turner@users.noreply.github.com>

hugovk added 6 commits September 25, 2023 18:39

Run pre-commit as part of 'make patchcheck'

dff437b

Use f-strings

97c6ccf

Add whitespace

029a9a4

Properly pluralise file(s), fix Unix typo

d7690a3

Remove normalize_docs_whitespace from patchcheck, it's covered by pre…

5cfcd19

…-commit's trailing-whitespace

Include inc files in pre-commit's trailing-whitespace

1eb8042

hugovk requested a review from AlexWaygood as a code owner September 25, 2023 15:52

bedevere-app Bot added the awaiting review label Sep 25, 2023

bedevere-app Bot mentioned this pull request Sep 25, 2023

Move Azure Pipelines CI to GitHub Actions #109408

Closed

hugovk added the skip news label Sep 25, 2023

AlexWaygood reviewed Sep 25, 2023

View reviewed changes

Comment thread Tools/patchcheck/patchcheck.py Outdated

Fix string

a85cd1c

Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com>

AlexWaygood approved these changes Sep 25, 2023

View reviewed changes

bedevere-app Bot added awaiting core review and removed awaiting review labels Sep 25, 2023

AA-Turner reviewed Sep 25, 2023

View reviewed changes

hugovk and others added 2 commits September 25, 2023 12:35

Use "s" variable for readability

433153d

Co-authored-by: Adam Turner <9087854+AA-Turner@users.noreply.github.com>

Reinstate docs_modified

a429344

AA-Turner reviewed Sep 26, 2023

View reviewed changes

Comment thread Tools/patchcheck/patchcheck.py

sobolevn reviewed Sep 26, 2023

View reviewed changes

Reinstate doc_files for docs_modified

63c6d17

Co-authored-by: Adam Turner <9087854+AA-Turner@users.noreply.github.com>

This was referenced Sep 26, 2023

gh-109408: Run patchcheck in GitHub Actions #109459

Closed

gh-109408: Stop running patchcheck in CI #109895

Merged

Revert makefile changes

05a5d35

hugovk changed the title gh-109408: Docs: move trailing-whitespace from patchcheck to pre-commit gh-109408: Move the docs whitespace check from patchcheck to pre-commit Sep 27, 2023

hugovk changed the title gh-109408: Move the docs whitespace check from patchcheck to pre-commit gh-109408: Add the docs whitespace check from patchcheck to pre-commit Oct 10, 2023

Merge branch 'main' into patchcheck-docs-whitespace

f4b16ec

hugovk enabled auto-merge (squash) October 10, 2023 07:45

hugovk added needs backport to 3.11 only security fixes needs backport to 3.12 only security fixes labels Oct 10, 2023

hugovk merged commit 7426ed0 into python:main Oct 10, 2023

hugovk deleted the patchcheck-docs-whitespace branch October 10, 2023 08:11

bedevere-app Bot removed the awaiting core review label Oct 10, 2023

bedevere-app Bot removed the needs backport to 3.12 only security fixes label Oct 10, 2023

bedevere-app Bot removed the needs backport to 3.11 only security fixes label Oct 10, 2023

hugovk mentioned this pull request Oct 10, 2023

gh-107652: Fix CIFuzz build #110576

Merged

		@$(RUNSHARED) ./$(BUILDPYTHON) -m pre_commit --version >/dev/null 2>&1 \|\| $(RUNSHARED) ./$(BUILDPYTHON) -m pip install pre-commit >/dev/null 2>&1
		$(RUNSHARED) ./$(BUILDPYTHON) -m pre_commit run --all-files

gh-109408: Add the docs whitespace check from patchcheck to pre-commit by hugovk · Pull Request #109854 · python/cpython · GitHub

Uh oh!

Uh oh!

gh-109408: Add the docs whitespace check from patchcheck to pre-commit - #109854

gh-109408: Add the docs whitespace check from patchcheck to pre-commit#109854hugovk merged 12 commits intopython:mainpython/cpython:mainfrom hugovk:patchcheck-docs-whitespacehugovk/cpython:patchcheck-docs-whitespaceCopy head branch name to clipboard

Conversation

hugovk commented Sep 25, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

1.

2.

3.

4.

5.

Uh oh!

Uh oh!

AlexWaygood left a comment

Choose a reason for hiding this comment

Uh oh!

AA-Turner Sep 25, 2023

Choose a reason for hiding this comment

Uh oh!

hugovk Sep 25, 2023

Choose a reason for hiding this comment

Uh oh!

AA-Turner Sep 26, 2023

Choose a reason for hiding this comment

Uh oh!

hugovk Sep 27, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

sobolevn left a comment

Choose a reason for hiding this comment

Uh oh!

AA-Turner commented Sep 26, 2023

Uh oh!

AlexWaygood commented Sep 26, 2023

Uh oh!

AA-Turner commented Sep 26, 2023

Uh oh!

miss-islington commented Oct 10, 2023

Uh oh!

bedevere-app Bot commented Oct 10, 2023

Uh oh!

bedevere-app Bot commented Oct 10, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

gh-109408: Add the docs whitespace check from patchcheck to pre-commit#109854
hugovk merged 12 commits into
python:mainfrom
hugovk:patchcheck-docs-whitespace

hugovk commented Sep 25, 2023 •
edited
Loading