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

Bump Ruff to 0.8.0, ignoring RUF022/RUF023 #13090

Merged
merged 13 commits into from
Nov 25, 2024

Conversation

Avasam
Copy link
Collaborator

@Avasam Avasam commented Nov 24, 2024

No manual fixes needed. I split the PR into 2 initial commits to help understand and evaluate the changes:

  1. Manual config changes (bump ruff and remove a now-redundant pyi-only ignore)
  2. Run ruff check --fix
  3. Run ruff format Forgot we use black 🙃

Now for the question: Should typeshed be re-ordering __all__ ? Or aim to follow the runtime's ordering?

This comment has been minimized.

@tungol
Copy link
Contributor

tungol commented Nov 25, 2024

I think it's probably fine to sort __all__ in typeshed. Without something in stubtest to check, we're just all over the place anyway, and any stubtest check would need to account for how much we have version-specific adding things which will usually get tacked at the end.

@AlexWaygood
Copy link
Member

Now for the question: Should typeshed be re-ordering __all__ ? Or aim to follow the runtime's ordering?

I can see arguments both ways, but could we split that discussion into a separate PR? That way we can see clearly which changes are specifically due to the RUF022 autofix; as it currently is, it's pretty hard to see if there are any other autofixes making changes here. I think what I'd like is:

  1. One PR bumping our Ruff pin to 0.8 and adding RUF022 to the list of ignored rules. We should hopefully be able to merge that pretty quickly.
  2. Another PR removing RUF022 from the list of ignored rules again; we can discuss on that PR whether RUF022 is appropriate for stub files or not

@Avasam Avasam changed the title Bump Ruff to 0.8.0 Bump Ruff to 0.8.0, ignoring RUF022 Nov 25, 2024
pyproject.toml Outdated Show resolved Hide resolved
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.

LGTM. Though I suppose we could also split RUF023 into the followup PR as well, since it's unlikely we'll want to auto-sort __slots__ but not __all__ at the end of the day. The same principles apply to both, we just have more __all__ definitions in the stub right now (but there have been proposals to add more __slots__ definitions in the past, and I can see the merits of doing so!)

This comment has been minimized.

@Avasam
Copy link
Collaborator Author

Avasam commented Nov 25, 2024

Though I suppose we could also split RUF023 into the followup PR as well, since it's unlikely we'll want to auto-sort __slots__ but not __all__ at the end of the day. The same principles apply to both, we just have more __all__ definitions in the stub right now

Fair. Since you already approved this PR, do you want me to also exclude the __slots__ change ? Or are you saying it can be reverted based on the decision in #13108 ?

@AlexWaygood
Copy link
Member

I would weakly prefer the __slots__ change to be discussed at the same time as the __all__ change, and done in the same PR as the __all__ change. But I also don't feel strongly, and am personally content for you to merge this PR straight away if you prefer -- hence my approval :-)

@Avasam
Copy link
Collaborator Author

Avasam commented Nov 25, 2024

It feels like trying to slip in a change. And I'd rather avoid the (small) risk we just forget about it. In hindsight it's pretty much the same discussion, so let's do them together, I'll revert that change from this PR.

pyproject.toml Outdated Show resolved Hide resolved
@Avasam Avasam changed the title Bump Ruff to 0.8.0, ignoring RUF022 Bump Ruff to 0.8.0, ignoring RUF022/RUF023 Nov 25, 2024
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.

And now you have my unqualified approval ;)

Thank you!

Copy link
Contributor

According to mypy_primer, this change has no effect on the checked open source code. 🤖🎉

@AlexWaygood AlexWaygood merged commit 69e3eb8 into python:main Nov 25, 2024
77 of 78 checks passed
@Avasam Avasam deleted the Bump-Ruff-to-0.8.0 branch November 25, 2024 18:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants