-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
PEP 764: Inlined typed dictionaries #4082
Conversation
Thank you for opening this PR. PEPs require a core developer or PEP editor who is willing to sponsor it, which your PR currently lacks. I would advise you to start a thread on the https://discuss.python.org/ forum to see if there is support for this proposal and if someone is willing to sponsor it. A |
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.
Thanks for writing this. From my perspective, it's looking pretty good.
I added some comments and questions for your consideration.
peps/pep-9995.rst
Outdated
don't have a name. For this reason, their :attr:`~type.__name__` attribute | ||
will be set to an empty string. | ||
|
||
It is not possible to specify any class arguments such as ``total``. |
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 think that Required
, NotRequired
and ReadOnly
should be permitted here. Absent any mention of this, the typing spec could be interpreted as disallowing this. It's better to make it clear.
I presume that all inlined TypedDicts are implicitly "total" (i.e. all items are assumed to be Required
unless explicitly NotRequired
). This makes sense because it's consistent with precedent, but it would be good to spell it out in the spec.
I think it's also worth considering making all inlined TypedDicts implicitly "closed" (as defined in draft PEP 728). This would deviate from precedent, but I think one could make a good argument for why inlined TypedDicts should be closed.
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 think that
Required
,NotRequired
andReadOnly
should be permitted here. Absent any mention of this, the typing spec could be interpreted as disallowing this. It's better to make it clear.I presume that all inlined TypedDicts are implicitly "total" (i.e. all items are assumed to be
Required
unless explicitlyNotRequired
). This makes sense because it's consistent with precedent, but it would be good to spell it out in the spec.
Updated.
I think it's also worth considering making all inlined TypedDicts implicitly "closed" (as defined in draft PEP 728). This would deviate from precedent, but I think one could make a good argument for why inlined TypedDicts should be closed.
Sounds reasonable. I have to say I'm still a bit confused with PEP 728 and closed=True
. If we disallow subclassing inlined typed dictionaries, it means that every inlined typed dictionary is implicitly @final
. What's the purpose of having closed=True
then? I guess I should ask my question in the PEP 728 discussion thread instead.
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 asked my question here: https://discuss.python.org/t/45443/138
and added a section in the open issues regarding closed behavior of inlined typed dictionaries
peps/pep-9995.rst
Outdated
don't have a name. For this reason, their :attr:`~type.__name__` attribute | ||
will be set to an empty string. | ||
|
||
It is not possible to specify any class arguments such as ``total``. |
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.
Am I correct in assuming that there is no way for an inlined TypedDict to "extend" another (named) TypedDict? One could imagine supporting this using dictionary expansion syntax, but that adds complexity that may not be justifiable. It would also impose additional runtime requirements on TypedDict classes (they'd need to be iterable).
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.
A good way to support inheritance could be to allow more than one argument to the subscript: TypedDict[Base, {"a": int}]
.
peps/pep-9995.rst
Outdated
parametrization overloads. On ther other hand, :class:`~typing.TypedDict` is | ||
already a :term:`special form <typing:special form>`. | ||
|
||
* If fufure work extends what inlined typed dictionaries can do, we don't have |
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.
Did you consider reusing typing.Dict
here? I can see some potential advantages. It's less verbose, doesn't have the baggage of builtins.dict
, and the fact that this is a "typed" dict is already implicit given that it is imported from typing
and involves type annotations.
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.
Showing my TypeScript bias here 😅 but I'm wondering how sacrilegious it would be to avoid the subscription all together and simply use the dictionary itself?
def get_movie() -> {'name': str, 'year': int}:
return {
'name': 'Blade Runner',
'year': 1982,
}
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.
This causes problems with the |
operator at least. I've been thinking of writing up something like "Why can't we ...?" discussing why certain "obvious" ways to make typing more ergonomic may not work well in practice.
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 think that such writeup would be very useful!
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 an extra section in the rejected ideas regarding the plain dict syntax.
On using typing.Dict
, I kind of like the idea for the reasons you mentioned @erictraut. Dict[{...}]
implicitly spells out as something like:
Dict[{...}]
| |------> ...with some typed fields
|--> a dict...
However, I still think this is debatable for a couple reasons:
typing.Dict
is currently marked as deprecated in the Python documentation (although not scheduled for removal). It might be confusing to undeprecate it.- The same subscripting syntax — with the only difference being the number of type arguments — means two different things. Can be confusing as well.
I'll add an entry to the open issues so that we can further discuss this option.
@Viicos, as a member of the typing council, I'm willing to sponsor this PEP. |
Reopened as Eric offered to sponsor (thanks Eric!). |
I just realized I opened this PR on the original repository, but meant to open it on my fork as part of this discussion. But I'm happy to keep going with this PEP. Regardless of the chosen implementation for partial, pick, etc, I can see how inlined typed dictionaries can be useful ( Thanks for the feedback. Should I open a separate discussion on the forum? Should it be in the PEPs/Typing category? |
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 just realized I opened this PR on the original repository, but meant to open it on my fork as part of this discussion.
Whichever you prefer :) Probably depends on how much more work you think it needs before you're ready to submit this to be merged as a draft.
Should I open a separate discussion on the forum? Should it be in the PEPs/Typing category?
Wait until this draft is merged, then open a new discussion in the Typing category so people can discuss the merged and proposed draft (see https://peps.python.org/pep-0001/#discussing-a-pep).
Also please review PEP 1 and PEP 12 for the PEP process.
I've updated the OP to include the new PEP checklist, please work through it and check off things as appropriate. Thanks!
Some GH discussions are left opened for now, waiting for an answer.
b4ba0c1
to
76dc942
Compare
2386695
to
2e0c379
Compare
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.
@Viicos, awesome job here! So exciting to see this awesome contribution!!
Although it is not possible to specify any class arguments such as ``total``, | ||
any :external+typing:term:`type qualifier` can be used for individual fields:: | ||
|
||
Movie = TypedDict[{'name': NotRequired[str], 'year': ReadOnly[int]}] |
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 wonder, is there a world in which eventually you could do something like:
Movie = Annotated[TypedDict[{'name': str, 'year': int}], Total(False)]
Or something like that for class args?
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.
This would require:
- introducing a
typing.Total
class/function - setting a precedent regarding the meaning of
Annotated
metadata, which is currently ignored by type checkers.
While using :class:`dict` isn't ideal, we could make use of | ||
:class:`typing.Dict` with a single argument:: | ||
|
||
def get_movie() -> Dict[{'title': str}]: ... |
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.
Hmm, I like the strict parametrization of TypedDict
better, I feel like this would set an odd precedent and confuse some users.
Apologies for the small (now undone) edit of one of the checkmarks, I was reading the PR and pushed the wrong button 😅 |
From the perspective of this user of type annotations, I have only one reservation: isn't this encouraging an antipattern? In my mind, if you need to type-annotate a tree-like dictionary structure, then isn't it time to come up with a tree of classes or named tuples or any other data structure to make it explicit? I would think that a With a complex tree-like dict, wouldn't it make more sense to add |
regardless of what your opinion is on whether or not type Foo = TypedDict[{"foo bar": int}] (yes i know you can also do another point i'd like to make is that the current class "syntax" for defining |
e9712cf
to
b3c1892
Compare
I believe this is ready to be merged and to be discussed on Discourse. |
Co-authored-by: Jelle Zijlstra <[email protected]>
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.
Left some feedback but I think it's in good shape to merge; we can change it later.
@erictraut as the sponsor could you confirm you're also OK with merging the PEP as it stands?
T2 = TypedDict[{'a': int}] | ||
|
||
As inlined typed dictionaries are are meant to be *anonymous*, their | ||
:attr:`~type.__name__` attribute will be set to an empty string. |
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.
That is going to be confusing, you'll get behavior like this:
>>> t = type("", (), {})
>>> t
<class '__main__.'>
>>> t()
<__main__. object at 0x104af1df0>
I'd set it to something like "<anonymous TypedDict>"
.
class SubTD(InlinedTD): | ||
pass | ||
|
||
What about defining an inlined typed dictionay extending another typed |
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 think we should do this, seems intuitive and useful.
This is somewhat arbitrary, and an alternative name could be used as well | ||
(e.g. ``'<TypedDict>'``). | ||
|
||
Alternatively, inlined typed dictionaries could be defined as instances of a |
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.
This might actually be better. Runtime introspection tools would likely already need some updates, or they'll output really confusing diagnostics for a TypedDict with an empty name. (Though I just tried and Pydantic doesn't seem to output the TypedDict name in its error message, so maybe it would be fine for you.)
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.
as the sponsor could you confirm you're also OK with merging the PEP as it stands?
Yes, I'm good with merging the latest draft.
Basic requirements (all PEP Types)
pep-NNNN.rst
), PR title (PEP 123: <Title of PEP>
) andPEP
headerAuthor
orSponsor
, and formally confirmed their approvalAuthor
,Status
(Draft
),Type
andCreated
headers filled out correctlyPEP-Delegate
,Topic
,Requires
andReplaces
headers completed if appropriate.github/CODEOWNERS
for the PEPStandards Track requirements
Python-Version
set to valid (pre-beta) future Python version, if relevantDiscussions-To
andPost-History
📚 Documentation preview 📚: https://pep-previews--4082.org.readthedocs.build/pep-0764/