GH-109408: Move the C file whitespace check from patchcheck to pre-commit #109890

AA-Turner · 2023-09-26T10:10:15Z

Issue: Move Azure Pipelines CI to GitHub Actions #109408

AlexWaygood

On the one hand, the patchcheck script will currently autofix this issue for you if run it locally, and it seems a shame to delete that functionality without adding it to pre-commit. On the other hand, I like having this check as part of pre-commit, and I also somewhat doubt that many people are trying to commit C source code that uses tabs in it into CPython -- so maybe the ability to autofix tabs isn't actually that useful after all.

AA-Turner · 2023-09-26T13:17:30Z

We could change it to run python -c "python -c "import sys; n=(t:=(f:=open(sys.argv[1],'r+b')).read()).replace(b'\t',b' '*8); f.seek(0); f.write(n); sys.exit(t!=n)" as a 'system' script (or add a script in Tools/scripts to achieve the same effect) -- I believe that this would alter the source files if run locally.

A

AlexWaygood · 2023-09-26T13:22:38Z

We could change it to run python -c "python -c "import sys; n=(t:=(f:=open(sys.argv[1],'r+b')).read()).replace(b'\t',b' '*8); f.seek(0); f.write(n); sys.exit(t!=n)" as a 'system' script (or add a script in Tools/scripts to achieve the same effect) -- I believe that this would alter the source files if run locally.

A

I vote for the latter -- code golf is fun but pretty unmaintainable ;)

AA-Turner · 2023-09-26T13:33:25Z

I vote for the latter -- code golf is fun but pretty unmaintainable ;)

Done, I've just reused untabify, which is what we currently use -- wins all around!

A

hugovk · 2023-09-26T15:00:43Z

Tools/patchcheck/untabify.py



 if __name__ == '__main__':
-    main()
+    raise SystemExit(main())


process() now has return values, which go to main(), but main() always returns None so we never get any errors here.

Oh, I didn't realise! pre-commit was failing in my local testing when I introduced tabs, perhaps it just fails when a file is changed after the hook has run.

hugovk · 2023-09-26T15:05:29Z

.pre-commit-config.yaml

+        name: "Check C file whitespace"
+        entry: "python Tools/patchcheck/untabify.py"
+        language: "system"
+        types_or: ['c', 'c++']


We were running on .c and .h files before:

c_files = [fn for fn in file_paths if fn.endswith(('.c', '.h'))]

Both of those are matched by the c type, so this would be closer to parity:

Suggested change

types_or: ['c', 'c++']

types_or: [c]

We do have half a dozen .cpp files in the codebase, do we want to expand to include them? Should we also add c++ for trailing-whitespace above?

The test passed with 'c++' included, I imagined that it was a previous oversight that they weren't included. I'll check if the trailing whitespace check also passes.

Tools/msi/bundle/bootstrap/PythonBootstrapperApplication.cpp fails with 16 lines changed. Other than that all good (@zooba would you be alright with us enabling the trailing whitespace check here? No real views either way, if you'd prefer to keep the whitespace then that's the status quo anyway!)

A

(We can always remove c++ later if needed.)

.pre-commit-config.yaml

hugovk · 2023-09-26T15:12:04Z

Tools/patchcheck/patchcheck.py

+    if has_c_files:
+        print("Did you run the test suite and check for refleaks?")
+    elif python_files:
+        print("Did you run the test suite?")


It's so much clearer like this! 👍

.pre-commit-config.yaml

hugovk · 2023-10-10T08:51:09Z

@AA-Turner Looks good, please could you resolve the conflict?

# Conflicts: # Tools/patchcheck/patchcheck.py

hugovk

Thanks!

miss-islington · 2023-10-10T13:41:14Z

Thanks @AA-Turner for the PR, and @hugovk for merging it 🌮🎉.. I'm working now to backport this PR to: 3.12.
🐍🍒⛏🤖

miss-islington · 2023-10-10T13:41:14Z

Thanks @AA-Turner for the PR, and @hugovk for merging it 🌮🎉.. I'm working now to backport this PR to: 3.11.
🐍🍒⛏🤖

miss-islington · 2023-10-10T13:41:18Z

Sorry, @AA-Turner and @hugovk, I could not cleanly backport this to 3.12 due to a conflict.
Please backport using cherry_picker on command line.

cherry_picker f5edb56328b46f262b74a53343b8098a3934f761 3.12

miss-islington · 2023-10-10T13:41:21Z

Sorry, @AA-Turner and @hugovk, I could not cleanly backport this to 3.11 due to a conflict.
Please backport using cherry_picker on command line.

cherry_picker f5edb56328b46f262b74a53343b8098a3934f761 3.11

…eck to pre-commit (pythonGH-109890) Co-authored-by: Hugo van Kemenade <[email protected]> (cherry picked from commit f5edb56) Co-authored-by: Adam Turner <[email protected]>

bedevere-app · 2023-10-10T14:31:23Z

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

…eck to pre-commit (pythonGH-109890) Co-authored-by: Hugo van Kemenade <[email protected]>. (cherry picked from commit f5edb56) Co-authored-by: Adam Turner <[email protected]>

bedevere-app · 2023-10-10T14:40:08Z

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

… pre-commit (GH-109890) (#110636) (cherry picked from commit f5edb56)

…pre-commit (python#109890) Co-authored-by: Hugo van Kemenade <[email protected]>

Move the C file whitespace check to pre-commit

f7d8e99

AA-Turner requested review from hugovk and AlexWaygood as code owners September 26, 2023 10:10

bedevere-app bot added the awaiting review label Sep 26, 2023

bedevere-app bot mentioned this pull request Sep 26, 2023

Move Azure Pipelines CI to GitHub Actions #109408

Closed

unused variable

25688d9

AA-Turner added the skip news label Sep 26, 2023

Exclude vendored code

6e9040c

AA-Turner changed the title ~~gh-109408: Move the C file whitespace check from patchcheck to pre-commit~~ GH-109408: Move the C file whitespace check from patchcheck to pre-commit Sep 26, 2023

This was referenced Sep 26, 2023

gh-109408: Run patchcheck in GitHub Actions #109459

Closed

gh-109408: Stop running patchcheck in CI #109895

Merged

AlexWaygood reviewed Sep 26, 2023

View reviewed changes

Fix tabs when run locally

0e20e9f

hugovk reviewed Sep 26, 2023

View reviewed changes

AA-Turner added 2 commits September 26, 2023 16:36

Exit with the right code

b44f4c2

zlib is no more

d4d31ff

hugovk reviewed Oct 10, 2023

View reviewed changes

.pre-commit-config.yaml Outdated Show resolved Hide resolved

More concise comment

b70cece

AA-Turner added 4 commits October 10, 2023 12:14

Merge branch 'main' into patchcheck/c-file-whitespace

986f2a3

# Conflicts: # Tools/patchcheck/patchcheck.py

Remove the now-redundant --ci option

b15ef27

Remove unused imports, variables, and functions

556978b

Verbose regular expression

6891bdb

hugovk approved these changes Oct 10, 2023

View reviewed changes

bedevere-app bot added awaiting core review and removed awaiting review labels Oct 10, 2023

hugovk merged commit f5edb56 into python:main Oct 10, 2023

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

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

miss-islington assigned hugovk Oct 10, 2023

hugovk mentioned this pull request Oct 10, 2023

GH-109408: Move the Python file whitespace check from patchcheck to pre-commit #109891

Merged

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

AlexWaygood pushed a commit that referenced this pull request Oct 10, 2023

[3.12] GH-109408: Move the C file whitespace check from patchcheck to…

c9157fe

… pre-commit (GH-109890) (#110636) (cherry picked from commit f5edb56)

hugovk mentioned this pull request Oct 11, 2023

gh-109408: Revert pre-commit whitespace checks pending portable solution #110726

Merged

Glyphack pushed a commit to Glyphack/cpython that referenced this pull request Sep 2, 2024

pythonGH-109408: Move the C file whitespace check from patchcheck to …

9e03c53

…pre-commit (python#109890) Co-authored-by: Hugo van Kemenade <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GH-109408: Move the C file whitespace check from patchcheck to pre-commit #109890

GH-109408: Move the C file whitespace check from patchcheck to pre-commit #109890

AA-Turner commented Sep 26, 2023 •

edited by bedevere-app bot

Loading

AlexWaygood left a comment

AA-Turner commented Sep 26, 2023

AlexWaygood commented Sep 26, 2023

AA-Turner commented Sep 26, 2023

hugovk Sep 26, 2023

AA-Turner Sep 26, 2023

hugovk Sep 26, 2023

AA-Turner Sep 26, 2023

AA-Turner Sep 26, 2023 •

edited

Loading

hugovk Oct 10, 2023

hugovk Sep 26, 2023

hugovk commented Oct 10, 2023

hugovk left a comment

miss-islington commented Oct 10, 2023

miss-islington commented Oct 10, 2023

miss-islington commented Oct 10, 2023

miss-islington commented Oct 10, 2023

bedevere-app bot commented Oct 10, 2023

bedevere-app bot commented Oct 10, 2023

GH-109408: Move the C file whitespace check from patchcheck to pre-commit #109890

GH-109408: Move the C file whitespace check from patchcheck to pre-commit #109890

Conversation

AA-Turner commented Sep 26, 2023 • edited by bedevere-app bot Loading

AlexWaygood 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

hugovk Sep 26, 2023

Choose a reason for hiding this comment

AA-Turner Sep 26, 2023

Choose a reason for hiding this comment

hugovk Sep 26, 2023

Choose a reason for hiding this comment

AA-Turner Sep 26, 2023

Choose a reason for hiding this comment

AA-Turner Sep 26, 2023 • edited Loading

Choose a reason for hiding this comment

hugovk Oct 10, 2023

Choose a reason for hiding this comment

hugovk Sep 26, 2023

Choose a reason for hiding this comment

hugovk commented Oct 10, 2023

hugovk left a comment

Choose a reason for hiding this comment

miss-islington commented Oct 10, 2023

miss-islington commented Oct 10, 2023

miss-islington commented Oct 10, 2023

miss-islington commented Oct 10, 2023

bedevere-app bot commented Oct 10, 2023

bedevere-app bot commented Oct 10, 2023

AA-Turner commented Sep 26, 2023 •

edited by bedevere-app bot

Loading

AA-Turner Sep 26, 2023 •

edited

Loading