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

[ruff] Unnecessary rounding (RUF057) #14828

Merged
merged 5 commits into from
Jan 2, 2025
Merged

Conversation

InSyncWithFoo
Copy link
Contributor

Summary

Resolves #14793.

Test Plan

cargo nextest run and cargo insta test.

Copy link
Contributor

github-actions bot commented Dec 7, 2024

ruff-ecosystem results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

✅ ecosystem check detected no linter changes.

@MichaReiser
Copy link
Member

MichaReiser commented Dec 7, 2024

Thanks for following up on this!

Would you mind splitting the PR into two if it isn't too much work:

  1. Changes the existing rule
  2. Adding the new rule

Two separate PRs have the advantage that we can list the two changes separately in the changelog. It also makes it easier for me to review your PR

@InSyncWithFoo
Copy link
Contributor Author

InSyncWithFoo commented Dec 7, 2024

@MichaReiser I would prefer not having to do that. RUF057 depends on a RUF046 API (expr_is_strictly_int()), which itself comprises most of the change to RUF046.

@MichaReiser
Copy link
Member

MichaReiser commented Dec 7, 2024

Yeah, that's a bit annoying. However, I just skimmed over the changes, and there's too much going on in this PR, which makes it difficult for me and future readers to understand what it is about. That's why I have to insist that you split the PR. I suggest three PRs:

  1. Scope change of RUF046
    2.. Improving the integer inference of RUF046
  2. Adding the new RUF057

I'd suggest you start with one PR and then submit the next once that is merged. Or you can create stacked PRs, but they can be a bit painful when it comes to rebasing them.

@InSyncWithFoo InSyncWithFoo changed the title [ruff] Detect more strict-integer expressions and move round() logic to new rule (RUF046, RUF057) [ruff] Unnecessary ndigits argument to round() (RUF057) Dec 8, 2024
@InSyncWithFoo InSyncWithFoo changed the title [ruff] Unnecessary ndigits argument to round() (RUF057) [ruff] Unnecessary ndigits argument to round() (RUF057) Dec 8, 2024
@InSyncWithFoo
Copy link
Contributor Author

All done. I don't mind a little bit of rebasing, so stacked PRs it is.

@MichaReiser
Copy link
Member

MichaReiser commented Dec 8, 2024

Thank you.

Hmm. This is not the rule I expected. From our conversation in #14793 I'd expected a rule that detects unnecessary round calls rather than unnecessary ndigits.

E.g. round(5) should be converted to just 5. The same for round(5, None)

MichaReiser pushed a commit that referenced this pull request Dec 9, 2024
## Summary

Part 1 of the big change introduced in #14828. This temporarily causes
all fixes for `round(...)` to be considered unsafe, but they will
eventually be enhanced.

## Test Plan

`cargo nextest run` and `cargo insta test`.
@InSyncWithFoo InSyncWithFoo changed the title [ruff] Unnecessary ndigits argument to round() (RUF057) [ruff] Unnecessary rounding (RUF057) Dec 25, 2024
@MichaReiser MichaReiser added rule Implementing or modifying a lint rule preview Related to preview mode features labels Dec 26, 2024
Copy link
Member

@MichaReiser MichaReiser left a comment

Choose a reason for hiding this comment

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

Thanks and happy new year. This is looking great. I've one last comment on where I think the rule should flag a round call but currently isn't. I'm interested in why you chose to not flag that round call site.

@MichaReiser MichaReiser merged commit 89ea037 into astral-sh:main Jan 2, 2025
21 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
preview Related to preview mode features rule Implementing or modifying a lint rule
Projects
None yet
Development

Successfully merging this pull request may close these issues.

RUF046 is doing two things instead of one
2 participants