-
-
Notifications
You must be signed in to change notification settings - Fork 31k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
GH-103699: Add __orig_bases__
to various typing classes
#103698
Conversation
__orig_bases__
to non-generic TypedDict
dba5772
to
7a81c98
Compare
__orig_bases__
to non-generic TypedDict__orig_bases__
to non-generic TypedDict
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you! Will wait for a second thumbs-up from a typing maintainer before merging, but LGTM
Lib/test/test_typing.py
Outdated
@@ -7126,6 +7126,9 @@ class TD(TypedDict): | |||
self.assertIs(type(a), dict) | |||
self.assertEqual(a, {'a': 1}) | |||
|
|||
def test_orig_bases(self): | |||
self.assertEqual(ChildTotalMovie.__orig_bases__, (ParentNontotalMovie,)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you add some more tests? Suggestions: a TypedDict with no bases, one with multiple bases, a generic TypedDict.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
__orig_bases__
on generic TypedDicts are already tested quite extensively, e.g.
cpython/Lib/test/test_typing.py
Line 7017 in 4037df1
self.assertEqual(A.__orig_bases__, (TypedDict, Generic[T])) |
Tests for TypedDicts with no bases and multiple bases sound worth adding, though
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added quite a few :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should I remove the checks on generic TypedDicts then? Or move them all here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should I remove the checks on generic TypedDicts then? Or move them all here?
I think it's probably fine as is, actually. A little bit of duplication in tests isn't a massive issue, and it makes sense to both:
1). Test __orig_bases__
in the test method for testing generic TypedDicts
2). Test generic TypedDicts in the test method for testing __orig_bases__
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, thanks!
I might want documentation for this attribute, but we have another PR open to add typing.get_original_bases
, so we can cover TypedDicts there too.
Hmm, one edge case that isn't currently covered in the tests is call-based >>> from typing import TypedDict
>>> class Foo(TypedDict): ...
...
>>> Foo2 = TypedDict("Foo2", {})
>>> Foo.__orig_bases__
(<function TypedDict at 0x000002796B986410>,)
>>> Foo2.__orig_bases__
() Here's the current behaviour with call-based >>> from typing import NamedTuple
>>> class Bar(NamedTuple): ...
...
>>> Bar2 = NamedTuple("Bar2", {})
>>> Bar.__orig_bases__
(<function NamedTuple at 0x000002796B986200>,)
>>> Bar2.__orig_bases__
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
AttributeError: type object 'Bar2' has no attribute '__orig_bases__' |
We could match the behaviour of diff --git a/Lib/typing.py b/Lib/typing.py
index 8408edc49a..660e89cb30 100644
--- a/Lib/typing.py
+++ b/Lib/typing.py
@@ -3107,7 +3107,9 @@ class body be required.
# Setting correct module is necessary to make typed dict classes pickleable.
ns['__module__'] = module
- return _TypedDictMeta(typename, (), ns, total=total)
+ td = _TypedDictMeta(typename, (), ns, total=total)
+ del td.__orig_bases__
+ return td
_TypedDict = type.__new__(_TypedDictMeta, 'TypedDict', (), {})
TypedDict.__mro_entries__ = lambda bases: (_TypedDict,) |
I'm happy to do that, but I think the new behavior is better. Even better would be if the call approaches matched the class-based approaches, but certainly the attribute always existing is better than it only existing sometimes, right? |
I think the conclusion from the in-person discussion was that we should make it return |
Yes, that sounds good. But we should fix it for call-based |
Yup that’s what I recall as well. Alex you did bring up the point about inline TypedDicts (which is being discussed in the mailing list and it seems like has a good chance of moving to a PEP), and how it might not make sense to have TypedDict in the bases for inline TypedDicts. Philosophically, I agree with that. But in practice, for the things that are going to be digging into the bases of a TypedDict, it is not a problem to have or not have TypedDict itself in the bases. So I think we should move forward with this keeping it as is to minimize changes and deal with inline TypedDicts if/when they show up since even if they behave different it wouldn’t really be a huge deal (and we can probably just get them to behave the same if we want to). |
I think @hauntsaninja brought up the point about inline TypedDicts, but I agree with your conclusion here! |
I don't know exactly how inline TypedDicts will work (for one, they won't have a name); that'll be up to Nikita to specify in the PEP. But yeah, we can discuss that when the time comes to actually implement it. |
Okay, applying this patch to your PR branch appears to make call-based diff --git a/Lib/typing.py b/Lib/typing.py
index 8408edc49a..354bc80eb3 100644
--- a/Lib/typing.py
+++ b/Lib/typing.py
@@ -2962,7 +2962,9 @@ class Employee(NamedTuple):
elif kwargs:
raise TypeError("Either list of fields or keywords"
" can be provided to NamedTuple, not both")
- return _make_nmtuple(typename, fields, module=_caller())
+ nt = _make_nmtuple(typename, fields, module=_caller())
+ nt.__orig_bases__ = (NamedTuple,)
+ return nt
_NamedTuple = type.__new__(NamedTupleMeta, 'NamedTuple', (), {})
@@ -3107,7 +3109,9 @@ class body be required.
# Setting correct module is necessary to make typed dict classes pickleable.
ns['__module__'] = module
- return _TypedDictMeta(typename, (), ns, total=total)
+ td = _TypedDictMeta(typename, (), ns, total=total)
+ td.__orig_bases__ = (TypedDict,)
+ return td
_TypedDict = type.__new__(_TypedDictMeta, 'TypedDict', (), {})
TypedDict.__mro_entries__ = lambda bases: (_TypedDict,) (Needs tests as well) |
Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com>
This, uh, would introduce a discrepancy between call-based >>> import collections as c, typing as t
>>> class Foo(c.namedtuple('Foo', {})): ...
...
>>> class Bar(t.NamedTuple('Bar', {})): ...
...
>>> Foo.__orig_bases__
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
AttributeError: type object 'Foo' has no attribute '__orig_bases__'
>>> Bar.__orig_bases__
(<function NamedTuple at 0x000001426A124730>,) But I think we can live with that... |
Misc/NEWS.d/next/Library/2023-04-22-22-37-39.gh-issue-103699.NizCjc.rst
Outdated
Show resolved
Hide resolved
…izCjc.rst Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com>
I added tests for call-based TypedDict and various NamedTuple scenarios in 631d428, your patch works great, thank you. I previously pushed 7224ddd but then remembered that NamedTuple inheritance is not really a thing.
I agree, especially given that (1) it can probably be fixed there as well and (2) I think we all agree that the new behavior is better. |
__orig_bases__
to non-generic TypedDict__orig_bases__
to various typing classes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
It surprises me that Bar.__orig_bases__
in this case is (NamedTuple,)
, since the "original bases" of Bar
are not (NamedTuple,)
(they're (Foo,)
):
>>> from typing import NamedTuple
>>> class Foo(NamedTuple): ...
...
>>> class Bar(Foo): ...
...
>>> Bar.__orig_bases__
(<function NamedTuple at 0x0000016E1F4ACEA0>,)
However, this behaviour is perfectly consistent with how inheriting from Generic[T]
works:
>>> from typing import Generic, TypeVar
>>> T = TypeVar("T")
>>> class Baz(Generic[T]): ...
...
>>> class Eggs(Baz): ...
...
>>> Eggs.__orig_bases__
(typing.Generic[~T],)
So I think it's fine.
Misc/NEWS.d/next/Library/2023-04-22-22-37-39.gh-issue-103699.NizCjc.rst
Outdated
Show resolved
Hide resolved
…izCjc.rst Co-authored-by: Jelle Zijlstra <jelle.zijlstra@gmail.com>
As discussed at the typing summit, non generic TypedDict's completely loose all inheritance information. Funny enough, generic TypedDict's actually preserve it via
__orig_bases__
because Generic does that. So this just brings non-generic TypedDicts to be the same.__orig_bases__
to non-generic TypedDict #103699