GH-109408: Move the C file whitespace check from patchcheck to pre-commit by AA-Turner · Pull Request #109890 · python/cpython · GitHub

Issue: Move Azure Pipelines CI to GitHub Actions #109408

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.

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

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 ;)

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

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.

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.)

It's so much clearer like this! 👍

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

# Conflicts: # Tools/patchcheck/patchcheck.py

Thanks!

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

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

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

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 <hugovk@users.noreply.github.com> (cherry picked from commit f5edb56) Co-authored-by: Adam Turner <9087854+AA-Turner@users.noreply.github.com>

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 <hugovk@users.noreply.github.com>. (cherry picked from commit f5edb56) Co-authored-by: Adam Turner <9087854+AA-Turner@users.noreply.github.com>

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 <hugovk@users.noreply.github.com>

Move the C file whitespace check to pre-commit

f7d8e99

AA-Turner requested review from AlexWaygood and hugovk 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

Comment thread .pre-commit-config.yaml Outdated

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 only 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 only 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 <hugovk@users.noreply.github.com>

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

+                  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?")

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

GH-109408: Move the C file whitespace check from patchcheck to pre-commit by AA-Turner · Pull Request #109890 · python/cpython · GitHub

Uh oh!

Uh oh!

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#109890hugovk merged 11 commits intopython:mainpython/cpython:mainfrom AA-Turner:patchcheck/c-file-whitespaceAA-Turner/cpython:patchcheck/c-file-whitespaceCopy head branch name to clipboard

Conversation

AA-Turner commented Sep 26, 2023 • edited by bedevere-app Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AlexWaygood 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!

hugovk Sep 26, 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 26, 2023

Choose a reason for hiding this comment

Uh oh!

AA-Turner Sep 26, 2023

Choose a reason for hiding this comment

Uh oh!

AA-Turner Sep 26, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hugovk Oct 10, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

hugovk Sep 26, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

hugovk commented Oct 10, 2023

Uh oh!

hugovk left a comment

Choose a reason for hiding this comment

Uh oh!

miss-islington commented Oct 10, 2023

Uh oh!

miss-islington commented Oct 10, 2023

Uh oh!

miss-islington commented Oct 10, 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!

4 participants

GH-109408: Move the C file whitespace check from patchcheck to pre-commit#109890
hugovk merged 11 commits into
python:mainfrom
AA-Turner:patchcheck/c-file-whitespace

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

AA-Turner Sep 26, 2023 •
edited
Loading