bpo-43783: add ParamSpecArgs/Kwargs#25298
Conversation
|
The CI failure looks unrelated to my changes: https://dev.azure.com/Python/cpython/_build/results?buildId=78089&view=logs&j=4db1505a-29e5-5cc0-240b-53a8a2681f75&t=a975920c-8356-5388-147c-613d5fab0171 |
Fidget-Spinner
left a comment
There was a problem hiding this comment.
This LGTM, with some minor notes about docs below. Thanks for doing this!
Co-authored-by: Ken Jin <28750310+Fidget-Spinner@users.noreply.github.com>
Co-authored-by: Ken Jin <28750310+Fidget-Spinner@users.noreply.github.com>
Co-authored-by: Ken Jin <28750310+Fidget-Spinner@users.noreply.github.com>
Co-authored-by: Ken Jin <28750310+Fidget-Spinner@users.noreply.github.com>
|
Thanks! I applied your suggestions. |
It seems it's fixed on the latest master commits. Try rebasing against new changes in master. |
|
Just merged master, hopefully everything will be green this time. |
| assert get_origin(P.args) is P | ||
| assert get_origin(P.kwargs) is P |
There was a problem hiding this comment.
While I know the idiom is commonly used and technically correct, formulating an equivalence using an assert does not feel clear to me. I'd much rather show it in a REPL fragment, state the equivalency in English, or show it in a comment, e.g.
get_origin(P.args) # returns P
| self.assertEqual(repr(P_co.args), "+P_co.args") | ||
| self.assertEqual(repr(P_co.kwargs), "+P_co.kwargs") |
There was a problem hiding this comment.
Oh, I didn't realize ParamSpec uses + to show it's covariant. It's clever, although a bit of an innovation for an edge case. It suddenly strikes me as slightly inappropriate here, since this is parsed as +(P_co.args) whereas the meaning is closer to (+P_co).args. But I wouldn't want to introduce those parentheses. Perhaps the variance of the args or kwargs attribute doesn't matter, and the repr() should not include the ~, + or - sign? I don't think this applies for TypeVar, since it has no attributes.
There was a problem hiding this comment.
It does look a bit weird here, I'll change the repr()s to include only the name. The variance of ParamSpecs has no defined meaning anyway, and even if it did it's not essential for it to be reflected here.
I'll push the remaining changes manually. Co-authored-by: Guido van Rossum <guido@python.org>
|
Thanks Guido for the review! I addressed your comments. |
From python/cpython#25298. I also added more tests for get_args/get_origin, which previously didn't exist in in typing_extensions.
From python/cpython#25298. I also added more tests for get_args/get_origin, which previously didn't exist in in typing_extensions.
From python/cpython#25298. I also added more tests for get_args/get_origin, which previously didn't exist in in typing_extensions.
From python/cpython#25298. I also added more tests for get_args/get_origin, which previously didn't exist in in typing_extensions.
This makes the
.argsand.kwargsattributes ofParamSpecobjects introspectable at runtime. Before this PR, these attributes are justobject()instances. This arose out of discussion here.I chose to create two new classes, instead of a single class with an attribute set to "args" or "kwargs", because args and kwargs are the only possible attributes. It feels cleaner to make illegal cases impossible to represent than to use a magical string attribute.
https://bugs.python.org/issue43783