Start PEP 728 implementation #519
Conversation
Co-authored-by: Daraan <github.blurry@9ox.net>
|
OK this is finally ready for review. |
|
A few days ago I made a PR #1 against your fork branch. Addressing this statement from the PEP:
|
|
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. |
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.
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.
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 |
| @@ -877,6 +878,59 @@ def inner(func): | |||
| return inner | |||
|
|
|||
|
|
|||
| 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.
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