[ruff] Unnecessary rounding (RUF057)#14828
Conversation
|
|
Thanks for following up on this! Would you mind splitting the PR into two if it isn't too much work:
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 |
|
@MichaReiser I would prefer not having to do that. |
|
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:
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. |
ruff] Detect more strict-integer expressions and move round() logic to new rule (RUF046, RUF057)ruff] Unnecessary ndigits argument to round() (RUF057)
ruff] Unnecessary ndigits argument to round() (RUF057)ruff] Unnecessary ndigits argument to round() (RUF057)
|
All done. I don't mind a little bit of rebasing, so stacked PRs it is. |
|
Thank you. Hmm. This is not the rule I expected. From our conversation in #14793 I'd expected a rule that detects unnecessary E.g. |
c446340 to
c8f9191
Compare
ruff] Unnecessary ndigits argument to round() (RUF057)ruff] Unnecessary rounding (RUF057)
crates/ruff_linter/src/rules/ruff/rules/unnecessary_rounding.rs
Outdated
Show resolved
Hide resolved
crates/ruff_linter/src/rules/ruff/rules/unnecessary_rounding.rs
Outdated
Show resolved
Hide resolved
crates/ruff_linter/src/rules/ruff/rules/unnecessary_rounding.rs
Outdated
Show resolved
Hide resolved
crates/ruff_linter/src/rules/ruff/rules/unnecessary_rounding.rs
Outdated
Show resolved
Hide resolved
929eed8 to
c0b6c2d
Compare
346b224 to
1dd112c
Compare
MichaReiser
left a comment
There was a problem hiding this comment.
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.
Summary
Resolves #14793.
Test Plan
cargo nextest runandcargo insta test.