bpo-46195: Do not add `Optional` in `get_type_hints` by sobolevn · Pull Request #30304 · python/cpython
A note about commit messages / PR titles: something does something is ambiguous because it could be describing wrong behaviour that the code did before the change, or new correct behaviour. Similar to the docstring convention (method columnize: Display a list of blah by blah), a good commit message or PR title should show what the changes do:
- Fix extra space added by columnize (better than: columnize adds extra space (a bug))
- Add extra space in columnize (better than: columnize adds extra space (a feature))
- columnize now adds extra space when needed (same — present tense +
nowavoids ambiguity)
So for this PR (if I understand current title correctly) it could be:
- Do not add Optional in get_type_hints
- Make get_type_hints not add Optional
sobolevn
changed the title
bpo-46195:
bpo-46195: Do not add get_type_hints does not add Optional anymoreOptional in get_type_hints
@merwok thanks, it is way more readable now! Wording (and writing in general) in English is something I really try to improve.
@Zac-HD do we rely on this behavior in hypothesis? 🤔
@Zac-HD do we rely on this behavior in
hypothesis? 🤔
It's certainly observable by our users, and would probably make some of our tests fail.
That said, I support the more consistent design proposed in the BPO issue, and we'll be fine carrying another clause in our type hints wrapper in Hypothesis. Probably important to reference the relevant PEP (section of 484, I think?) in the changelog though.
I have production code that currently assumes parameters that default to None are implicitly Optional; the required changes to accommodate this are trivial on my part, and I can certainly plan within the 3.11 time horizon.
A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.
Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In my opinion this should be 3.11+ only 🤔
Yeah. This is definitely a breaking change so it should be 3.11+ only. I wonder how many runtime checkers are affected or even use get_type_hints for that matter? @samuelcolvin can I trouble you for an opinion on this please? @Tinche too for cattrs too please. Thank you.
In short, typing.get_type_hints automatically wraps a type annotation with Optional if it sees a default None. This PR proposes to cease that behavior.
In my opinion this should be 3.11+ only 🤔
Yeah. This is definitely a breaking change so it should be 3.11+ only. I wonder how many runtime checkers are affected or even use
get_type_hintsfor that matter? @samuelcolvin can I trouble you for an opinion on this please? @Tinche too for cattrs too please. Thank you.In short,
typing.get_type_hintsautomatically wraps a type annotation withOptionalif it sees a defaultNone. This PR proposes to cease that behavior.
@Fidget-Spinner I'm totally fine with this, correct change in my opinion.
Just out of curiosity, does this change only apply to function parameters or class members too?
@Tinche for classes Optional was never added implictly (which is another point why we should fix this inconsistency).
Here are two examples, main and 3.9:
» ./python.exe Python 3.11.0a3+ (heads/main:e13cdca0f5, Jan 11 2022, 12:43:49) [Clang 11.0.0 (clang-1100.0.33.16)] on darwin Type "help", "copyright", "credits" or "license" for more information. >>> import typing >>> class A: ... x: int = None ... >>> typing.get_type_hints(A) {'x': <class 'int'>}
» python Python 3.9.9 (main, Dec 21 2021, 11:35:28) [Clang 11.0.0 (clang-1100.0.33.16)] on darwin Type "help", "copyright", "credits" or "license" for more information. >>> import typing >>> class A: ... x: int = None ... >>> typing.get_type_hints(A) {'x': <class 'int'>}
However, I can't find a test case for this 🤔
@Tinche for classes
Optionalwas never added implictly (which is another point why we should fix this inconsistency).Here are two examples,
mainand3.9:» ./python.exe Python 3.11.0a3+ (heads/main:e13cdca0f5, Jan 11 2022, 12:43:49) [Clang 11.0.0 (clang-1100.0.33.16)] on darwin Type "help", "copyright", "credits" or "license" for more information. >>> import typing >>> class A: ... x: int = None ... >>> typing.get_type_hints(A) {'x': <class 'int'>}» python Python 3.9.9 (main, Dec 21 2021, 11:35:28) [Clang 11.0.0 (clang-1100.0.33.16)] on darwin Type "help", "copyright", "credits" or "license" for more information. >>> import typing >>> class A: ... x: int = None ... >>> typing.get_type_hints(A) {'x': <class 'int'>}However, I can't find a test case for this 🤔
Thanks for the info!
Thanks for the great question! 👍
I've added this corner case to our tests in a separate PR: #30535
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like this change, just a comment on the docs.
Comment on lines +2061 to +2062
| ``Optional`` annotation is not added implicitly anymore | ||
| when ``None`` default is used for a function argument. |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the wording in the removed docs better. Suggestion:
"Previously, Optional[t] was added for function and method annotations if a default value equal to None was set. Now the annotation is returned unchanged."
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great addition!
In my opinion this should be 3.11+ only thinking
Yeah. This is definitely a breaking change so it should be 3.11+ only. I wonder how many runtime checkers are affected or even use
get_type_hintsfor that matter? @samuelcolvin can I trouble you for an opinion on this please? @Tinche too for cattrs too please. Thank you.In short,
typing.get_type_hintsautomatically wraps a type annotation withOptionalif it sees a defaultNone. This PR proposes to cease that behavior.
Thanks @Fidget-Spinner for asking my opinion, really useful to get a heads up about things like this. I don't think it will affect pydantic, if it does we can take care of it when adding support for 3.11.
I'm happy with this change provided it's no back-ported to other versions of python.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@gvanrossum this is another one I'd like to merge.
@gpshead previously requested changes, but his request was addressed.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IIUC, @gpshead mist still approve it.
tk0miya added a commit to tk0miya/sphinx that referenced this pull request
Mar 2, 2022Since python-3.11, `typing.get_type_hints()` will not add Optional[t] to type annotations even if a default value for function argument is None. refs: python/cpython#30304 (bpo-46195)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters