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

RUF046 is doing two things instead of one #14793

Closed
InSyncWithFoo opened this issue Dec 5, 2024 · 4 comments · Fixed by #14828
Closed

RUF046 is doing two things instead of one #14793

InSyncWithFoo opened this issue Dec 5, 2024 · 4 comments · Fixed by #14828
Labels
rule Implementing or modifying a lint rule

Comments

@InSyncWithFoo
Copy link
Contributor

I know that 0.8.2 has just been released and that I did say everything looked fine to me before #14697 was merged, but the more I think about it, the more I find it wrong to handle both round() and int() in one fell swoop. I probably won't be able to sleep well tonight without first sharing my concerns, so here's the big proposal:

RUF046 should only handle cases where the int() call is unnecessary. The logic to detect round() calls with unnecessary second argument should be moved to a new rule.

This will also make it easier to extend RUF046 to handle expressions more complex than a simple call, as it won't need to care about the exact content.

@dylwil3 dylwil3 added rule Implementing or modifying a lint rule needs-decision Awaiting a decision from a maintainer labels Dec 5, 2024
@MichaReiser
Copy link
Member

I probably won't be able to sleep well tonight without first sharing my concerns, so here's the big proposal:

Oh no! Don't worry too much about it. The rule is in preview. We're still allowed to make changes to its intent.

RUF046 should only handle cases where the int() call is unnecessary. The logic to detect round() calls with unnecessary second argument should be moved to a new rule.

Can you tell me a bit more about it? Do you mean that the rule fixes int(round(4, 0)) to round(4)? And what you're suggesting is to introduce a new rule that detects unnecessary-round-call for round(int). The way int(round(4)) would then first be fixed to int(4) and then to just 4?

@MichaReiser MichaReiser removed the needs-decision Awaiting a decision from a maintainer label Dec 6, 2024
@InSyncWithFoo
Copy link
Contributor Author

@MichaReiser Yes, that's correct. There will be a expr_is_strictly_int() function that checks if an expression is strictly an integer, which can be utilized by both rules to determine fix safety.

@MichaReiser
Copy link
Member

The suggested split makes sense to me. So we'd have two rules:

  • unnecessary-cast-to-int (e.g. int(round(4.0)))
  • unnecessary-rounding (e.g. round(4))

Do you want to a) first simplify unnecessary-cast-to-int and then add a new rule for unnecessary-rounding?

@InSyncWithFoo
Copy link
Contributor Author

InSyncWithFoo commented Dec 6, 2024

(It's actually round(4, 0)/round(whatever, None).)

I intend to extend unnecessary-cast-to-int so that it can detect complex expressions at the same time I introduce unnecessary-round-ndigits (already named locally, changing is trivial).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
rule Implementing or modifying a lint rule
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants