GeezerVison

The World Wide Web, restored to 800 pixels of glory.

GeezerVison is showing github.com — GH-109408: Move the C file whitespace check from patchcheck to pre-commit by AA-Turner · Pull Request #109890 · python/cpython

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.

Copy link

Copy Markdown

Member

Issue: Move Azure Pipelines CI to GitHub Actions #109408

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions


          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

AlexWaygood left a comment

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

AA-Turner commented Sep 26, 2023

Copy link

Copy Markdown

Member Author

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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

AlexWaygood commented Sep 26, 2023

Copy link

Copy Markdown

Member

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

hugovk reacted with thumbs up emoji

All reactions

👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.


          Fix tabs when run locally

0e20e9f

AA-Turner commented Sep 26, 2023

Copy link

Copy Markdown

Member Author

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!

AlexWaygood and hugovk reacted with rocket emoji

All reactions

🚀 2 reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

hugovk reviewed

Sep 26, 2023

View reviewed changes

Comment thread

Tools/patchcheck/untabify.py


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

hugovk Sep 26, 2023

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

AA-Turner Sep 26, 2023

Copy link

Copy Markdown

Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

.pre-commit-config.yaml

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

hugovk Sep 26, 2023

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

AA-Turner Sep 26, 2023

Copy link

Copy Markdown

Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

AA-Turner Sep 26, 2023 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

hugovk Oct 10, 2023

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

.pre-commit-config.yaml Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread

Tools/patchcheck/patchcheck.py

Comment on lines +278 to +281

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

hugovk Sep 26, 2023

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's so much clearer like this! 👍

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

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 Resolved

Uh oh!

There was an error while loading. Please reload this page.


          More concise comment

b70cece

hugovk commented Oct 10, 2023

Copy link

Copy Markdown

Member

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

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

hugovk left a comment

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

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 commented Oct 10, 2023

Copy link

Copy Markdown

Contributor

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

miss-islington commented Oct 10, 2023

Copy link

Copy Markdown

Contributor

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

miss-islington commented Oct 10, 2023

Copy link

Copy Markdown

Contributor

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

miss-islington assigned hugovk

Oct 10, 2023

miss-islington commented Oct 10, 2023

Copy link

Copy Markdown

Contributor

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

hugovk mentioned this pull request

Oct 10, 2023

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

Merged

AA-Turner added a commit to AA-Turner/cpython that referenced this pull request

Oct 10, 2023


          [3.12] pythonGH-109408: Move the C file whitespace check from patchch…

ae0dd83

…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 Bot commented Oct 10, 2023

Copy link

Copy Markdown

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

bedevere-app Bot removed the needs backport to 3.12 only security fixes label

Oct 10, 2023

AA-Turner added a commit to AA-Turner/cpython that referenced this pull request

Oct 10, 2023


          [3.11] pythonGH-109408: Move the C file whitespace check from patchch…

fd8ddd4

…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 Bot commented Oct 10, 2023

Copy link

Copy Markdown

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

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]>

Reviewers

AlexWaygood

AlexWaygood left review comments

hugovk

hugovk approved these changes

Uh oh!

There was an error while loading. Please reload this page.

Assignees

hugovk

Labels

skip news

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

4 participants

Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.

You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.

GeezerVison

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

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

AA-Turner commented Sep 26, 2023 •

edited by bedevere-app Bot

Loading

Uh oh!

There was an error while loading. Please reload this page.

AA-Turner Sep 26, 2023 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.