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

hugovk · 2023-09-25T15:52:12Z

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

Tools/patchcheck/patchcheck.py

Co-authored-by: Alex Waygood <[email protected]>

AlexWaygood

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

AA-Turner · 2023-09-25T17:58:01Z

Makefile.pre.in

+	@$(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 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

Tools/patchcheck/patchcheck.py

Co-authored-by: Adam Turner <[email protected]>

Tools/patchcheck/patchcheck.py

sobolevn

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 <[email protected]>

AA-Turner · 2023-09-26T10:13:43Z

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

AlexWaygood · 2023-09-26T11:35:47Z

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

AA-Turner · 2023-09-26T14:20:30Z

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

A

miss-islington · 2023-10-10T08:11:54Z

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 <[email protected]> Co-authored-by: Alex Waygood <[email protected]> Co-authored-by: Adam Turner <[email protected]>

bedevere-app · 2023-10-10T08:12:05Z

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

bedevere-app · 2023-10-10T08:12:09Z

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 <[email protected]> Co-authored-by: Alex Waygood <[email protected]> Co-authored-by: Adam Turner <[email protected]>

…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 <[email protected]> Co-authored-by: Alex Waygood <[email protected]> Co-authored-by: Adam Turner <[email protected]>

…-commit (python#109854) Co-authored-by: Alex Waygood <[email protected]> Co-authored-by: Adam Turner <[email protected]>

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

Tools/patchcheck/patchcheck.py Outdated Show resolved Hide resolved

Fix string

a85cd1c

Co-authored-by: Alex Waygood <[email protected]>

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 <[email protected]>

Reinstate docs_modified

a429344

AA-Turner reviewed Sep 26, 2023

View reviewed changes

Tools/patchcheck/patchcheck.py Outdated Show resolved Hide resolved

sobolevn reviewed Sep 26, 2023

View reviewed changes

Reinstate doc_files for docs_modified

63c6d17

Co-authored-by: Adam Turner <[email protected]>

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 bug and security fixes labels Oct 10, 2023

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

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 bug and 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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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 #109854

hugovk commented Sep 25, 2023 •

edited

Loading

AlexWaygood left a comment

AA-Turner Sep 25, 2023

hugovk Sep 25, 2023

AA-Turner Sep 26, 2023

hugovk Sep 27, 2023

sobolevn left a comment

AA-Turner commented Sep 26, 2023

AlexWaygood commented Sep 26, 2023

AA-Turner commented Sep 26, 2023

miss-islington commented Oct 10, 2023

bedevere-app bot commented Oct 10, 2023

bedevere-app bot commented Oct 10, 2023

		@$(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 #109854

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

Conversation

hugovk commented Sep 25, 2023 • edited Loading

1.

2.

3.

4.

5.

AlexWaygood left a comment

Choose a reason for hiding this comment

AA-Turner Sep 25, 2023

Choose a reason for hiding this comment

hugovk Sep 25, 2023

Choose a reason for hiding this comment

AA-Turner Sep 26, 2023

Choose a reason for hiding this comment

hugovk Sep 27, 2023

Choose a reason for hiding this comment

sobolevn left a comment

Choose a reason for hiding this comment

AA-Turner commented Sep 26, 2023

AlexWaygood commented Sep 26, 2023

AA-Turner commented Sep 26, 2023

miss-islington commented Oct 10, 2023

bedevere-app bot commented Oct 10, 2023

bedevere-app bot commented Oct 10, 2023

hugovk commented Sep 25, 2023 •

edited

Loading