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

Update the target Python version for Ruff commands #26094

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

deepyaman
Copy link
Contributor

@deepyaman deepyaman commented Nov 22, 2024

Summary & Motivation

Fix a minor inconsistency. There's no effect, because most pyupgrade rules aren't included.

(I'd be happy to introduce the pyupgrade rules, if people are open to it.)

@deepyaman deepyaman marked this pull request as ready for review November 22, 2024 02:07
@danielgafni
Copy link
Contributor

danielgafni commented Nov 22, 2024

@deepyaman hey, looks like ruff actually wants to reformat some files.

❯ make ruff
ruff check --fix .
All checks passed!
ruff format .
27 files reformatted, 4359 files left unchanged

It wasn't kicked off in CI, probably because of the lack of changes in this PR.

As for the UP rule, I am already working on this here: #25703, so I'd suggest to keep the current scope of this PR.

@danielgafni danielgafni self-requested a review November 22, 2024 15:13
Copy link
Contributor

@danielgafni danielgafni left a comment

Choose a reason for hiding this comment

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

Please run make ruff

@deepyaman
Copy link
Contributor Author

@deepyaman hey, looks like ruff actually wants to reformat some files.

❯ make ruff
ruff check --fix .
All checks passed!
ruff format .
27 files reformatted, 4359 files left unchanged

It wasn't kicked off in CI, probably because of the lack of changes in this PR.

Got it. Pre-commit was actually making these changes for me, but I didn't commit them, since I thought something was wrong with my Ruff setup. 🙈 I didn't understand why upgrading the target version would make these changes; I thought maybe Ruff wasn't picking up line-length configuration or something on my end, but seems not.

As for the UP rule, I am already working on this here: #25703, so I'd suggest to keep the current scope of this PR.

Ah, I didn't see this! I've gone ahead and made the requested change, but if you want to just close my PR in favor of yours, that's also understandable!

@danielgafni
Copy link
Contributor

It's fine, there shouldn't be conflicts with my PR! Or they are going to be easy to resolve.

Copy link
Contributor

@danielgafni danielgafni left a comment

Choose a reason for hiding this comment

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

lgtm

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.

2 participants