[flake8-pyi] Implement autofix for redundant-numeric-union (PYI041)#14273
Conversation
|
| code | total | + violation | - violation | + fix | - fix |
|---|---|---|---|---|---|
| PYI041 | 70 | 0 | 4 | 66 | 0 |
flake8-pyi] Implement autofix and handle nested unions with single element (PYI041 , PYI055)
#14214
MichaReiser
left a comment
There was a problem hiding this comment.
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
|
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.
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.
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.
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.
I tweaked the comment a bit on all three rules.
…c_union.rs Co-authored-by: Micha Reiser <micha@reiser.io>
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 <micha@reiser.io> Co-authored-by: Charlie Marsh <charlie.r.marsh@gmail.com>
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