Skip to content
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

Support __extra__ for PEP 728. #329

Merged
merged 3 commits into from
Feb 18, 2024
Merged

Support __extra__ for PEP 728. #329

merged 3 commits into from
Feb 18, 2024

Conversation

PIG208
Copy link
Contributor

@PIG208 PIG208 commented Feb 8, 2024

No description provided.

@PIG208
Copy link
Contributor Author

PIG208 commented Feb 8, 2024

I assume that I need to update doc/index.rst and CHANGELOG.md. Will get back to it later this day.

@PIG208 PIG208 force-pushed the extra branch 3 times, most recently from fea586b to b05b4a5 Compare February 9, 2024 06:11
@PIG208
Copy link
Contributor Author

PIG208 commented Feb 9, 2024

Added documentation on this feature. I used 4.10.0 as the next version number, as I think __extra__ wouldn't pose backwards compatibility issues given that dunder attributes are generally reserved.

@srittau srittau added the under-discussion Don't merge until an external discussion has been resolved label Feb 9, 2024
@srittau
Copy link
Collaborator

srittau commented Feb 9, 2024

Thanks! Currently, there's some discussion about this in https://discuss.python.org/t/pep-728-typeddict-with-typed-extra-items/45443/9. We should hold off merging this for a few days, until the discussion is resolved.

@JelleZijlstra
Copy link
Member

Agree. My plan is to wait for a week or so for both this one and PEP 742 to reach a rough consensus in the discussion, then make a new typing-extensions release for both.

@JelleZijlstra
Copy link
Member

@PIG208 What do you think of implementing Eric's proposal from https://discuss.python.org/t/pep-728-typeddict-with-typed-extra-items/45443/24 ? That seems the best option so far.

@JelleZijlstra
Copy link
Member

In particular, if we can get the implementation ready by Sunday, it can go into the upcoming release 4.10.0. If you don't have time in the next few days, I can work on an implementation tomorrow.

@PIG208
Copy link
Contributor Author

PIG208 commented Feb 16, 2024

I was working on updating the PEP. Will probably have both this implementation and the PEP update ready today. Thanks for notifying me!

@PIG208
Copy link
Contributor Author

PIG208 commented Feb 17, 2024

I'm thinking about if and how we should include the special "extra_item" for runtime introspection. Perhaps it is not ideal to leave it in __annotations__, because the regular __extra_item__ can also end up there and get potentially overridden in a child class that has the special __extra_item__ key.

Should we instead use an attribute __closed__ to host the annotated type when the special __extra_item__ is used? If that's the case, should we copy the value of __closed__ to the subclass during inheritance?

@PIG208
Copy link
Contributor Author

PIG208 commented Feb 17, 2024

An alternative would be defining __closed__ as a boolean indicating if the current TypedDict type is closed, and finding somewhere else to store the annotation type of the special extra item. This is similar to how __total__ works.

However, for introspection, __required_keys__ and __optional_keys__ are generally more appropriate for __total__, and its semantics could be confusing. So I'm not sure if the boolean flag will be necessary for PEP 728.

@JelleZijlstra
Copy link
Member

I think the general principle should be that we should allow runtime typing tools to reconstruct what the user wrote as much as possible, without deciding for them what the exact semantics are. After all, there may be edge cases (e.g. involving forward references) where the runtime typing tool is able to use more advanced logic than typing-extensions to figure out the right behavior. However, we have to balance that argument with creating an intuitive interface for users.

I do think we need two attributes: __closed__ matching the value of the closed= class argument, and __extra_keys__ with the annotation for the magic key __extra_keys__. If we didn't have those two keys, we couldn't distinguish between a class with __extra_keys__: None (weird but legal) and one with no __extra_keys__ at all.

I'd want the following semantics:

class TD1(TypedDict):
    a: int
assert TD1.__closed__ is False
assert TD1.__extra_keys__ is None
assert TD1.__annotations__ == {'a': int}

class TD2(TypedDict, closed=True):
    a: int
assert TD2.__closed__ is True
assert TD2.__extra_keys__ is Never
assert TD2.__annotations__ == {'a': int}

class TD3(TypedDict, closed=True):
    a: int
    __extra_keys__: str
assert TD3.__closed__ is True
assert TD3.__extra_keys__ is str
assert TD3.__annotations__ == {'a': int}

class TD4(TD2):
    b: int

assert TD4.__closed__ is True
assert TD4.__extra_keys__ is Never
assert TD4.__annotations__ == {'a': int, 'b': int}

class TD5(TD3):
    b: int

assert TD5.__closed__ is True
assert TD5.__extra_keys__ is str
assert TD5.__annotations__ == {'a': int, 'b': int}

class TD6(TD2, closed=False):  # resets inheritance of __extra_keys__
    b: int

assert TD6.__closed__ is False
assert TD6.__extra_keys__ is None
assert TD6.__annotations__ == {'a': int, 'b': int}

class TD7(TD3):
    __extra_keys__: str  # just a regular key

assert TD7.__closed__ is True
assert TD7.__extra_keys__ is int
assert TD7.__annotations__ == {'a': int, '__extra_keys__': str}

@PIG208
Copy link
Contributor Author

PIG208 commented Feb 17, 2024

It makes sense to have both dunder attributes, but I'm surprised by the "resetting inheritance of extra_keys" behavior.

I expected that __extra_keys__ always gets inherited and __closed__ depends only on the TypedDict type instead of its bases, like what __total__ currently does:

class TD6(TD2, closed=False):
    b: int

assert TD6.__closed__ is False
assert TD6.__extra_keys__ is Never  # instead of None
assert TD6.__annotations__ == {'a': int, 'b': int}

Was this decision driven by distinguishing TypedDict with __extra_keys__: None and one without __extra_keys__?

Edit: I think I got it now. When __closed__ is False, we can safely assume that __extra_keys__ == None indicates that all of the current TypedDict and its bases are non-closed. This leads to the resetting behavior when the keyword argument __closed__ conflicts with the inherited __closed__ attribute.

Signed-off-by: Zixuan James Li <p359101898@gmail.com>
@PIG208
Copy link
Contributor Author

PIG208 commented Feb 17, 2024

This current implementation inherits __extra_items__ from the bases (assuming there are no conflicts) and sets __closed__ only based on the current TypedDict type.

A drawback is that we have to avoid setting __extra_items__ on non-closed TypedDict types to distinguish missing__extra_items__ from __extra_items__: None.

Also reorganize the test cases and add coverage.

Signed-off-by: Zixuan James Li <p359101898@gmail.com>
@JelleZijlstra
Copy link
Member

Also a few lint errors in CI

@PIG208
Copy link
Contributor Author

PIG208 commented Feb 18, 2024

Hm, fixing the errors now.

This also fixes the lint errors.

Signed-off-by: Zixuan James Li <p359101898@gmail.com>
@JelleZijlstra JelleZijlstra merged commit b7bf949 into python:main Feb 18, 2024
18 checks passed
@PIG208 PIG208 deleted the extra branch February 18, 2024 00:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
under-discussion Don't merge until an external discussion has been resolved
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants