Skip to content

gh-132390: Apply Ruff linting to Tools/build #132391

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 46 commits into from
Apr 20, 2025

Conversation

picnixz
Copy link
Member

@picnixz picnixz commented Apr 11, 2025

@picnixz picnixz changed the title Lint/ruff/tools build fix 132390 gh-132390: Lint Tools/build with existing ruff configuration Apr 11, 2025
@picnixz
Copy link
Member Author

picnixz commented Apr 11, 2025

@AA-Turner @hugovk Does it look acceptable or would you prefer to just exclude the files instead of ruffing them?

@picnixz picnixz enabled auto-merge (squash) April 12, 2025 07:32
@picnixz picnixz disabled auto-merge April 12, 2025 07:34
@picnixz
Copy link
Member Author

picnixz commented Apr 12, 2025

I'll actually hold the merge a bit more so that code owners can also give their opinion.

@picnixz picnixz requested review from tomasr8 and hugovk April 13, 2025 09:16
Copy link
Member

@AlexWaygood AlexWaygood left a comment

Choose a reason for hiding this comment

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

Looks like a nice cleanup to me, but I'm not the person maintaining most of these scripts :-)

make sure to get signoff from the relevant maintainers before landing this. Not everybody on the team is necessarily on board with stricter linting in CI!

@picnixz
Copy link
Member Author

picnixz commented Apr 13, 2025

make sure to get signoff from the relevant maintainers before landing this. Not everybody on the team is necessarily on board with stricter linting in CI!

Yes, that's why I eventually decided to hold off the merge!

@picnixz
Copy link
Member Author

picnixz commented Apr 13, 2025

Ok so here is how I decided to make the change:

  • use f-strings instead of .format(); both have the same readability IMO.
  • do not use f-strings for %s when it's less readable. And if there are enough occurrences of %-usage, just ignore it for the file (namely generate_token.py)
  • if %-style and f-strings are roughly the same readability and the file already uses f-strings elsewhere, make everything f-strings (smelly.py)

Copy link
Contributor

@sethmlarson sethmlarson left a comment

Choose a reason for hiding this comment

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

LGTM for generate_sbom.py.

@picnixz picnixz self-assigned this Apr 19, 2025
@picnixz picnixz merged commit 5d8e432 into python:main Apr 20, 2025
46 checks passed
@picnixz picnixz deleted the lint/ruff/tools-build-fix-132390 branch April 20, 2025 09:21
@@ -455,7 +453,7 @@ def do_unixy_check(manifest, args):
okay &= _report_unexpected_items(
extra_defs,
'Some extra declarations were found in "Include/Python.h" '
+ 'with Py_LIMITED_API:')
'with Py_LIMITED_API:')
Copy link
Member

Choose a reason for hiding this comment

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

Here, I did want to point out that it's a single string and not a forgotten comma.
Could you point me to the reasoning behind this change?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants