| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
Sorry, something went wrong.
There was a problem hiding this comment.
I'm sorry, I didn't get to the end of this review (not by far), but I have a few comments. Main thing though is, did you look at all the examples in PEP 612? It seems that for user-defined generic classes that have a ParamSpec parameter, things like X[int, P] are actually valid.
Maybe this indirectly also answers your question: I think that the ParamSpec should be included in __parameters__.
Also, I wonder if we could do less type checking at runtime rather than trying to be strict? I desperately want typing.py to shrink, not grow... :-)
Sorry, something went wrong.
|
Points taken. Sorry X[int, P] slipped my mind since I hadn't fixed __parameters__ yet. D'oh, I just realised I answered my own question since we need __parameters__ to implement X[int, P] in the first place. |
Sorry, something went wrong.
Co-Authored-By: Guido van Rossum <gvanrossum@users.noreply.github.com>
also reduced runtime checks
|
Let's wait until #23060 has landed and then see what adjustments this might need. |
Sorry, something went wrong.
|
Finally looking at this...
Yeah, ParamSpec is super special, and you can't do anything with it that isn't explicitly mentioned in the PEP. So it's debatable whether Callable[P, int] should even have P in its __parameters__. Then again, for the simple case A = Callable[P, int] there's no reason why we couldn't allow A[[str, str]] and get Callable[[str, str], int] as the result. But type checkers wouldn't know what to do with that -- ParamSpec is a magic cookie. So let's not worry about those clearly wrong cases. |
Sorry, something went wrong.
|
Sorry, submitted too soon. (UPDATE: Not, I made a mistake -- or GitHub did. :-) |
Sorry, something went wrong.
There was a problem hiding this comment.
Excellent work! Thanks for your perseverance. My feedback is mostly simple improvements.
Sorry, something went wrong.
| } | ||
| // convert all the lists inside args to tuples to help | ||
| // with caching in other libaries if substituting a ParamSpec | ||
| if (PyList_CheckExact(subst) && is_paramspec(arg)) { |
There was a problem hiding this comment.
Why only do this for ParamSpec?
Sorry, something went wrong.
There was a problem hiding this comment.
I'm afraid of existing code relying on substituting typevars in genericalias with lists, then expecting that list to be inside __args__. Although in Python 3.9 that should be invalid, and the IDE/type checker should warn the users about that already.
If I just convert list to tuples for all cases, the C code will become way simpler :).
Sorry, something went wrong.
There was a problem hiding this comment.
There can't be much code yet relying about anything for GenericAlias -- it's new in 3.9. We could backport that particular behavior to 3.9.3 to ensure there won't be any more code relying on it. :-)
Sorry, something went wrong.
Co-Authored-By: Guido van Rossum <gvanrossum@users.noreply.github.com>
There was a problem hiding this comment.
Re: the 3.9 issues that you discovered: maybe create a PR for those first, land that in master and 3.9 branch, then merge master into this PR?
Sorry, something went wrong.
|
Guido, thank you so much for all the time you've put into reviewing this PR! I learnt quite a bit about the typing module in the process :).
Yeah I agree, I created #23915 for that. Once that one lands, I'll merge the changes in along with some of your suggestions. |
Sorry, something went wrong.
|
I think it’s ready! |
Sorry, something went wrong.
|
@Fidget-Spinner Let me know if you agree. |
Sorry, something went wrong.
Yes! Sorry, I thought you were going to merge it in so I was waiting too 😆 . |
Sorry, something went wrong.
|
I figured that was the case. :-) |
Sorry, something went wrong.
|
@gvanrossum: Please replace # with GH- in the commit message next time. Thanks! |
Sorry, something went wrong.
|
@gvanrossum Oh dang I knew I had a hunch I missed something :/. I forgot to convert all lists to tuples in ga_new/setup_ga too (I did it only for getitem and co.). Well on the bright side, having that as a separate PR makes backporting to 3.9 easier if anything :), and anyways lists aren't valid in genericalias other than the Callable case (which we already have covered), so we won't have anything broken even without that in right now. |
Sorry, something went wrong.
|
I'll fix that some other time, right now I'm celebrating 🥳 |
Sorry, something went wrong.
| Back | FazBrowse Home | New Git URL |
Changes:
Documentation will be another patch.
https://bugs.python.org/issue41559