-
-
Notifications
You must be signed in to change notification settings - Fork 2.9k
PEP 655: Support Required[] inside TypedDict #10370
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
Conversation
Looks like I have a CI failure to investigate... |
…eclare allow_required kwarg.
This is the same approach used with TypeGuardType, which RequiredType is modeled after.
CI has passed. Ready for folks to take a look. |
Outline of changes, to hopefully make the review easier:
|
I did a quick review pass and overall looks good. It's okay to move forward with supporting |
Thanks for the review @JukkaL ! I'll convert this PR to draft for now and add additional commits for the |
PR is extended with support for |
Would you be able to fix the merge conflict -- I can review this now. Apologies for the very slow response! |
I’ve been disconnected from open source for several months to take care of some life issues. I’m hoping I’ll have enough energy to reengage here in a month or so. In the meantime I welcome any rebase/deconflict support from any observers who’d like this branch to be merged faster. |
No problem! |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Diff from mypy_primer, showing the effect of this PR on open source code: steam.py (https://github.com/Gobot1234/steam.py)
- steam/iterators.py:70: error: Name "Required" is not defined [name-defined]
- steam/iterators.py:71: error: Name "Required" is not defined [name-defined]
- steam/trade.py:70: error: Variable "typing_extensions.Required" is not valid as a type [valid-type]
- steam/trade.py:70: note: See https://mypy.readthedocs.io/en/stable/common_issues.html#variables-vs-type-aliases
- steam/trade.py:71: error: Variable "typing_extensions.Required" is not valid as a type [valid-type]
- steam/trade.py:71: note: See https://mypy.readthedocs.io/en/stable/common_issues.html#variables-vs-type-aliases
- steam/trade.py:275: error: Argument 1 to "update" of "TypedDict" has incompatible type "AssetDict"; expected "TypedDict({'instanceid'?: Required?[builtins.str], 'classid'?: Required?[builtins.str], 'market_name'?: str, 'currency'?: int, 'name'?: str, 'market_hash_name'?: str, 'name_color'?: str, 'background_color'?: str, 'type'?: str, 'descriptions'?: Dict[str, str], 'market_actions'?: List[Dict[str, str]], 'tags'?: List[Dict[str, str]], 'actions'?: List[Dict[str, str]], 'icon_url'?: str, 'icon_url_large'?: str, 'tradable'?: bool, 'marketable'?: bool, 'commodity'?: int, 'fraudwarnings'?: List[str]})" [typeddict-item]
+ steam/trade.py:275: error: Argument 1 to "update" of "TypedDict" has incompatible type "AssetDict"; expected "TypedDict({'instanceid'?: str, 'classid'?: str, 'market_name'?: str, 'currency'?: int, 'name'?: str, 'market_hash_name'?: str, 'name_color'?: str, 'background_color'?: str, 'type'?: str, 'descriptions'?: Dict[str, str], 'market_actions'?: List[Dict[str, str]], 'tags'?: List[Dict[str, str]], 'actions'?: List[Dict[str, str]], 'icon_url'?: str, 'icon_url_large'?: str, 'tradable'?: bool, 'marketable'?: bool, 'commodity'?: int, 'fraudwarnings'?: List[str]})" [typeddict-item]
- steam/http.py:70: error: Name "Required" is not defined [name-defined]
- steam/http.py:71: error: Name "Required" is not defined [name-defined]
- steam/state.py:70: error: Name "Required" is not defined [name-defined]
- steam/state.py:71: error: Name "Required" is not defined [name-defined]
|
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 now. Thank you @davidfstr and @97littleleaf11! This has been a frequently requested feature.
@JukkaL Thanks for your review! And this PR also looks good to me. |
Thanks @97littleleaf11 for getting this branch finished while I've had to step back for a while! And thank you @JukkaL for getting it reviewed. Team work :) |
Adds support for the Required[] syntax inside TypedDicts as specified by PEP 655 (draft). NotRequired[] is also supported. Co-authored-by: 97littleleaf11 <[email protected]>
Description
Adds support for the
Required[]
syntax inside TypedDicts as specified by PEP 655. Note that theNotRequired[]
syntax from that PEP is not in this initial draft, because I want to see first if there is any feedback on how I'm implementingRequired[]
.A separate PR will be made to the
typing_extensions
module to enable runtime support forRequired[]
.CC sponsor @gvanrossum
Test Plan
New tests exist in
test-data/unit/check-typeddict.test
after the-- Required[]
line, and can be run individually with lines like:pytest -n0 mypy/test/testcheck.py::TypeCheckSuite::testRequiredOnlyAllowsOneItem