Uh oh!
There was an error while loading. Please reload this page.
Start PEP 728 implementation - #519
Conversation
Uh oh!
There was an error while loading. Please reload this page.
Co-authored-by: Daraan <github.blurry@9ox.net>
JelleZijlstra
commented
Feb 11, 2025
OK this is finally ready for review. |
Daraan
commented
Feb 11, 2025
A few days ago I made a PR #1 against your fork branch. Addressing this statement from the PEP:
|
Daraan
commented
Feb 12, 2025
I realized python/cpython#109544 is missing - if not hasattr(tp_dict, '__total__'):- tp_dict.__total__ = total+ tp_dict.__total__ = totalResulting in a different behavior when using it outside of the spec, i.e. it being the value and not True. |
Daraan
left a comment
There was a problem hiding this comment.
From my understanding, this example in the PEP should pass without any errors or warnings:
class BaseMovie(TypedDict, closed=True):
name: str
class MovieA(BaseMovie): # OK, still closed
pass
Currently this does raise a warning; so I think the relevant check should be closed is False (see suggestion below).
This will cause quite a few with self.assertWarns(DeprecationWarning): to fail afterwards which then need a look-over.
From another side should we even check this, or should this only be handled by type-checkers?
It is a type checker error to pass closed=False if a superclass has closed=True or sets extra_items.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Daraan
left a comment
There was a problem hiding this comment.
Looking at the runtime specifications once more. __extra_items__ currently should also reflect the constructor and closed=True => extra_items=Never is only implicitly implied/equivalent. Suggested the necessary changes for that.
Also, suggested the GrandGrandChild as an example of an implicit closed subclass. I think such a thing was still missing.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
JelleZijlstra
commented
Feb 22, 2025
Good catch, I guess when we made that change in CPython we didn't think about the fact that you could set something in the class body. Would you be interested in (1) adding a test case to CPython where there is an assignment to |
Is a new entry to the changelog necessary? Else looks good to me. (1) you have seen already, (2) added #533 |
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
| if not hasattr(typing, "NoDefault") or not hasattr(typing, "NoExtraItems"): | ||
| class SingletonMeta(type): |
There was a problem hiding this comment.
kind of surprised we bothered. tbh if someone wants to do type(NoDefault).asdf = 123 i could live with that
There was a problem hiding this comment.
I guess I did it initially for parity with the C version in CPython for NoDefault, where it's easier to not accept extra attributes, and now that I need another one it makes sense to follow the pattern.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com>
| if closed is not None and extra_items is not NoExtraItems: | ||
| raise TypeError(f"Cannot combine closed={closed!r} and extra_items") |
There was a problem hiding this comment.
I think this branch may also be uncovered, but feel free to merge and tackle this as a followup!
Early version but need to get this code off the laptop where it currently lives