-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
[flake8-pyi
] Implement autofix for redundant-numeric-union
(PYI041
)
#14273
[flake8-pyi
] Implement autofix for redundant-numeric-union
(PYI041
)
#14273
Conversation
crates/ruff_linter/src/rules/flake8_pyi/rules/redundant_numeric_union.rs
Show resolved
Hide resolved
crates/ruff_linter/src/rules/flake8_pyi/rules/redundant_numeric_union.rs
Show resolved
Hide resolved
crates/ruff_linter/src/rules/flake8_pyi/rules/redundant_numeric_union.rs
Show resolved
Hide resolved
crates/ruff_linter/src/rules/flake8_pyi/rules/redundant_numeric_union.rs
Outdated
Show resolved
Hide resolved
crates/ruff_linter/src/rules/flake8_pyi/rules/redundant_numeric_union.rs
Show resolved
Hide resolved
|
code | total | + violation | - violation | + fix | - fix |
---|---|---|---|---|---|
PYI041 | 70 | 0 | 4 | 66 | 0 |
crates/ruff_linter/src/rules/flake8_pyi/rules/redundant_numeric_union.rs
Outdated
Show resolved
Hide resolved
flake8-pyi
] Implement autofix and handle nested unions with single element (PYI041
, PYI055
)
#14214
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. This overall looks great.
The main open questions are
- should use text based edits to avoid reformatting unchanged code and loosing inner comments.
- Whether it's possible to always mark the fix as safe, regardless of comments
crates/ruff_linter/src/rules/flake8_pyi/rules/redundant_numeric_union.rs
Outdated
Show resolved
Hide resolved
crates/ruff_linter/src/rules/flake8_pyi/rules/redundant_numeric_union.rs
Outdated
Show resolved
Hide resolved
crates/ruff_linter/src/rules/flake8_pyi/rules/redundant_numeric_union.rs
Outdated
Show resolved
Hide resolved
crates/ruff_linter/src/rules/flake8_pyi/rules/redundant_numeric_union.rs
Show resolved
Hide resolved
crates/ruff_linter/src/rules/flake8_pyi/rules/redundant_numeric_union.rs
Show resolved
Hide resolved
crates/ruff_linter/src/rules/flake8_pyi/rules/redundant_numeric_union.rs
Outdated
Show resolved
Hide resolved
crates/ruff_linter/src/rules/flake8_pyi/rules/redundant_numeric_union.rs
Outdated
Show resolved
Hide resolved
Oh, and did you look through the ecosystem changes? If so, could you update the test plan. It's helpful to know if you did because I then don't need to carefully review all of them and can instead just click through some of them. |
The ecosystem results look good. Typeshed shows negative in the ecosystem results, but I expect that to not be related to the PR. |
Could you expand what you mean by it appears to skip that now. Is it not flagging this case or do you think this case shouldn't be flagged? Is it worth to add a test like that example (or that specific example) to our test suite? |
/// This rule's fix is marked as safe for most cases; however, the fix will | ||
/// flatten nested unions type expressions into a single top-level union. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reading the documentation, I get the impression the fix isn't safe because it flattens nested union expressions. But I understand that this doesn't make the fix unsafe, it's just an extension where the fix goes beyond what one might expect.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Exactly. I've reused the phrasing that Charlie used for another rule.
The nesting is a feature to allow cases such as these:
ReadOnlyMode = Literal["r", "r+"]
WriteAndTruncateMode = Literal["w", "w+", "wt", "w+t"]
WriteNoTruncateMode = Literal["r+", "r+t"]
AppendMode = Literal["a", "a+", "at", "a+t"]
AllModes = Literal[ReadOnlyMode, WriteAndTruncateMode,
WriteNoTruncateMode, AppendMode]
As a consequence this redundant nesting is also allowed, but can be safely fixed:
AllModes = Literal[Literal["r", "r+"], Literal["w", "w+", "wt", "w+t"],
Literal["r+", "r+t"], Literal["a", "a+", "at", "a+t"]]
See https://typing.readthedocs.io/en/latest/spec/literal.html#legal-and-illegal-parameterizations
We probably should have I will create a rule for this (for both union
and literal
).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After reading the linked document, I understand that both examples are valid. Can you share an example where flattening a nested union is unsafe?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tweaked the comment a bit on all three rules.
crates/ruff_linter/src/rules/flake8_pyi/rules/redundant_numeric_union.rs
Outdated
Show resolved
Hide resolved
crates/ruff_linter/src/rules/flake8_pyi/rules/redundant_numeric_union.rs
Show resolved
Hide resolved
crates/ruff_linter/src/rules/flake8_pyi/rules/redundant_numeric_union.rs
Show resolved
Hide resolved
…c_union.rs Co-authored-by: Micha Reiser <[email protected]>
The ecosystem reports for typeshed have often not been reproducible locally and nobody's yet had a chance to look into why... if you can't repro the typeshed hits locally, I'd just ignore them! |
After investigating this, it turned out that this rule hooked into |
4f75828
to
1af2c5f
Compare
…41`) (astral-sh#14273) ## Summary This PR adds autofix for `redundant-numeric-union` (`PYI041`) There are some comments below to explain the reasoning behind some choices that might help review. <!-- What's the purpose of the change? What does it do, and why? --> Resolves part of astral-sh#14185. ## Test Plan <!-- How was it tested? --> --------- Co-authored-by: Micha Reiser <[email protected]> Co-authored-by: Charlie Marsh <[email protected]>
Summary
This PR adds autofix for
redundant-numeric-union
(PYI041
)There are some comments below to explain the reasoning behind some choices that might help review.
Resolves part of #14185.
Test Plan