-
Notifications
You must be signed in to change notification settings - Fork 1.1k
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
[pyupgrade
] - add PEP646 Unpack conversion to *
with fix (UP044
)
#13988
Conversation
ee80d22
to
f52e85a
Compare
Nice, thanks. |
|
code | total | + violation | - violation | + fix | - fix |
---|---|---|---|---|---|
UP044 | 18 | 18 | 0 | 0 | 0 |
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.
This is great. Thank you!
Could you add one more test case for an unpacking where the target is a binary expression? This is is semantically invalid but it isn't a syntax error and we should at least avoid introducing a syntax error (I don't think we do, but we should make sure of it).
|
||
if checker.settings.preview.is_enabled() { | ||
diagnostic.set_fix(Fix::safe_edit(Edit::range_replacement( | ||
format!("*{inner}"), |
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.
Unpacking anything other than a Tuple
or TypeVarTuple
is semantically incorrect but it isn't a syntax error. For example:
def foo(*args: Unpack[int | str]) -> None:
Could we add a test asserting that the suggested fix doesn't suggest invalid syntax?
|
||
let inner = checker.locator().slice(slice.as_ref()); | ||
|
||
if checker.settings.preview.is_enabled() { |
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.
Why is this condition required when the rule itself is in preview?
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.
Silly me!
6bbcc0c
to
026ee0b
Compare
if slice.is_bin_op_expr() { | ||
// fixing this would introduce a syntax error | ||
return; | ||
} |
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.
Applying the fix seems "safe enough":
def foo(*args: *int | str) -> None: pass # not supported
Isn't invalid syntax
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.
ah, true, it gives a TypeError actually.
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 went ahead and converted it to an allowlist. I don't think any expressions other than name, subscript, and attribute are valid in this position.
It's giving SyntaxError when changing this: from typing import TypedDict, Unpack
class KWs(TypedDict):
"""kws."""
foo: str
def func(**kwargs: Unpack[KWs]) -> None: # <-- original
"""Func."""
_ = kwargs to this: from typing import TypedDict, Unpack
class KWs(TypedDict):
"""kws."""
foo: str
def func(**kwargs: *KWs) -> None: # <-- modified
"""Func."""
_ = kwargs |
Summary
Adds
Unpack[...]
->*...
for Py311+Closes #13891
Test Plan
cargo test