bpo-41559: Change PEP 612 implementation to pure Python #25449
Conversation
|
@rhettinger sorry to ping you; would you like me to remove your request for review for future typing-only changes to |
|
@gvanrossum Guido, I have the feeling that you're really busy with many review requests right now. So I apologise for the ping and I don't mean to rush. I just have a few pointers to help make your reviewing easier:
BTW, if you feel you're way too overwhelmed right now, maybe we could ping someone else (eg. Jelle?) to get this reviewed if they have more leeway? Thanks for your time. I hope you enjoy your well-deserved break after beta lands. |
|
It’s worse, am swamped with meetings and Thursday I am leaving for a 9 day
vacation. Can you find another reviewer?
On Tue, Apr 27, 2021 at 06:54 Ken Jin ***@***.***> wrote:
@gvanrossum <https://github.com/gvanrossum> Guido, I have the feeling
that you're really busy with many review requests right now. So I apologise
for the ping and I don't mean to rush. I just have a few pointers to help
make your reviewing easier:
- You can probably skim the C changes, they were created directly by git
checkout 463c7d3 -- genericaliasobject.c
(ie revert to older code), so unless you're unhappy with the old code,
there shouldn't be much work there.
- Most of the substitution code is a direct copy from typing.py 's
_GenericAlias., or a direct re-implementation of the C code in Python
with tweaks for ParamSpec.
BTW, if you feel you're way too overwhelmed right now, maybe we could ping
someone else (eg. Zelle?) to get this reviewed if they have more leeway?
Thanks for your time. I hope you enjoy your well-deserved break after beta
lands.
Off-topic: "Beta freeze" seems like a contradiction -- Nobody's feeling
cold and everyone's feeling the heat now 😅
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#25449 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAWCWMT5F6N3PEGMRZ2ESCTTK262TANCNFSM43CSGLBA>
.
--
--Guido (mobile)
|
|
No worries Guido. @JelleZijlstra, sorry may I summon you for a review please? |
|
I'll try to take a look by end of today. |
Thank you so much! |
There was a problem hiding this comment.
Mostly looks good. I have one suggestion for improving the code and I'd like to see some more tests.
It took me a while to realize this, but it looks like the observable effect of the change is that while on current master this works:
>>> P = ParamSpec("P")
>>> T = TypeVar("T")
>>> list[T][int]
list[int]
>>> list[P][int]
list[int]
This PR instead makes it so that P is not recognized as a TypeVar-like by the implementation of list[int], so the last line will instead throw TypeError: There are no type variables left in list[~P].
Jelle, thank you for reviewing this on such short notice. Yes that is indeed what I intended. Perhaps I wasn't clear enough in the news item and I should mention this is a breaking change from earlier versions of 3.10 alpha.
https://www.python.org/dev/peps/pep-0612/#valid-use-locations Re: the tests, I agree, I'll add them in soon. |
Co-Authored-By: Jelle Zijlstra <jelle.zijlstra@gmail.com>
|
Yay, the tests are now all green. @gvanrossum, Jelle has reviewed and approved this PR. Would you like for me to request for any additional reviewers? Edit: Oops sorry, I misspelled Jelle's name. |
|
@gvanrossum: Please replace |
|
Thanks Ken Jin for the PR, and thanks Jelle for the review!! |
|
My pleasure. A huge thanks to Jelle for the review (I don't think this could've landed without him) and thanks Guido for the merge! |
genericaliasobject.cto commit 463c7d3 (right before PEP 612) via a cleangit checkout 463c7d3d149283814d879a9bb8411af64e656c8e -- genericaliasobject.c.collections.abc.Callablein pure Python.PR 1/2, another update to typing.py is coming later, but that's not urgent.
The intention of this change is to conform more strictly to PEP 612. Specifically, this snippet:
https://www.python.org/dev/peps/pep-0612/#valid-use-locations
The current implementation leaks extra behavior into all builtin
GenericAliastypes just for the sake ofcollections.abc.Callable, which is out of the scope of the PEP. Since there is no need to supportParamSpecin other places, it's valid to not have them in__parameters__of all builtin generics. except forcollections.abc.Callable.This will also speed up builtin
GenericAliasas it has to do less checks, and reduce the amount of confusing code inGenericAlias(though it defers that tocollections.abc.Callable.)https://bugs.python.org/issue41559