Skip to content
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

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

Open
wants to merge 44 commits into
base: main
Choose a base branch
from

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?

@AA-Turner AA-Turner changed the title gh-132390: Lint Tools/build with existing ruff configuration gh-132390: Apply Ruff linting to Tools/build Apr 12, 2025
@picnixz picnixz marked this pull request as ready for review April 12, 2025 07:32
@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!

@@ -12,9 +12,22 @@ select = [
"LOG", # flake8-logging
"PGH", # pygrep-hooks
"PT", # flake8-pytest-style
"PYI", # flake8-pyi
Copy link
Member

Choose a reason for hiding this comment

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

why remove these from the select list? I like them (but I'm biased -- I wrote a lot of the lints originally as part of the flake8-pyi plugin 😆)

Copy link
Member Author

@picnixz picnixz Apr 13, 2025

Choose a reason for hiding this comment

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

This one recommends Set -> AbstractSet and namedtuple -> NamedTuple. Are there other interesting rules there that would be better enabled? (I don't think those two rules should be enforced so I ignored them but I think Adam just removed the PYI* rules altogether)

Copy link
Member

Choose a reason for hiding this comment

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

The first of those is only a stylistic lint. But it's a stylistic lint that I agree with -- it is confusing that collections.abc.Set has that name, since it means something very different to builtins.set or typing.Set.

The second of those is arguably a correctness rule if you're running a type checker in CI: it encourages you to use a version of namedtuple that gives type checkers much more information to work with.

You can see in flake8-pyi's original documentation that there are many lints in this category that are related to the correctness of annotations, not merely questions of style: https://github.com/PyCQA/flake8-pyi/blob/main/ERRORCODES.md. (I'm linking to the original flake8-pyi docs as we don't have a summary in Ruff's docs that's quite as convenient, but the implementation isn't very different at all in Ruff's version, so the information still applies.)

Since we run mypy on at least one of these scripts in CI (and we may start doing so with more of them in the future), I think lints that enforce correctness of type annotations are pretty useful:

- "Tools/build/generate_sbom.py"

Copy link
Member

Choose a reason for hiding this comment

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

(I don't think those two rules should be enforced so I ignored them but I think Adam just removed the PYI* rules altogether)

Ah yes, I see this is one of the changes @AA-Turner made in his commits to this PR branch

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

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.

6 participants