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] Do not simplify round() calls (RUF046) #14832

Merged
merged 5 commits into from
Dec 9, 2024

Conversation

InSyncWithFoo
Copy link
Contributor

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.

Copy link
Contributor

github-actions bot commented Dec 8, 2024

ruff-ecosystem results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

✅ ecosystem check detected no linter changes.

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 for splitting the PR!

I think the rule is now over eager in removing int calls from rount

@InSyncWithFoo
Copy link
Contributor Author

InSyncWithFoo commented Dec 8, 2024

For reference, here's the logic table for round() cases:

number ndigits round() Error/fix
Literal int Not given int Safe
Literal int Literal int int Safe
Literal int None int Safe
Literal int Other Other -
Literal float Not given int Safe
Literal float Literal int float -
Literal float None int Safe
Literal float Other Other -
Inferred int Not given int Unsafe
Inferred int Literal int int Unsafe
Inferred int None int Unsafe
Inferred int Other Other -
Inferred float Not given int Unsafe
Inferred float Literal int float -
Inferred float None int Unsafe
Inferred float Other Other -
Other Not given Likely int Unsafe
Other Literal int Other -
Other None Likely int Unsafe
Other Other Other -

@MichaReiser
Copy link
Member

MichaReiser commented Dec 9, 2024

Nice table: From testing a few cases, it seems that round(4, ndigits=10) always returns an int

>>> round(4, 10)
4
>>> type(_)
<class 'int'>

But we should not create a diagnostic if ndigits isn't an integer, e.g. round(4, 10.4)

@MichaReiser MichaReiser added rule Implementing or modifying a lint rule preview Related to preview mode features labels Dec 9, 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.

Nice. This looks good now, except that we can also provide a safe fix if the number is an int, and ndigits is any integer.

@InSyncWithFoo
Copy link
Contributor Author

Logic changed and table updated. round(0, integer) and round(inferred_integer, integer) are now both fixable, safely and unsafely correspondingly.

@MichaReiser
Copy link
Member

Perfect, thank you

@MichaReiser MichaReiser merged commit c62ba48 into astral-sh:main Dec 9, 2024
21 checks passed
@InSyncWithFoo InSyncWithFoo deleted the RUF046-1 branch December 9, 2024 16:39
dcreager added a commit that referenced this pull request Dec 10, 2024
* main:
  [`airflow`] Add fix to remove deprecated keyword arguments (`AIR302`) (#14887)
  Improve mdtests style (#14884)
  Reference `suppress-dummy-regex-options` in documentation of rules supporting it (#14888)
  [`flake8-bugbear`] `itertools.batched()` without explicit `strict` (`B911`) (#14408)
  [`ruff`] Mark autofix for `RUF052` as always unsafe (#14824)
  [red-knot] Improve type inference for except handlers (#14838)
  More typos found by codespell (#14880)
  [red-knot] move standalone expression_ty to TypeInferenceBuilder::file_expression_ty (#14879)
  [`ruff`] Do not simplify `round()` calls (`RUF046`) (#14832)
  Stop referring to early ruff versions (#14862)
  Fix a typo in `class.rs` (#14877)
  [`flake8-pyi`] Also remove `self` and `cls`'s annotation (`PYI034`) (#14801)
  [`pyupgrade`] Remove unreachable code in `UP015` implementation (#14871)
  [`flake8-bugbear`] Skip `B028` if `warnings.warn` is called with `*args` or `**kwargs` (#14870)
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.

2 participants