bpo-46195: Do not add Optional in get_type_hints#30304
Conversation
|
A note about commit messages / PR titles:
So for this PR (if I understand current title correctly) it could be:
|
Sorry, something went wrong.
|
@merwok thanks, it is way more readable now! Wording (and writing in general) in English is something I really try to improve. |
Sorry, something went wrong.
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. |
Sorry, something went wrong.
|
Will this be applied in Python 3.11, or is it intended to be back-ported to previous versions? |
Sorry, something went wrong.
|
In my opinion this should be 3.11+ only 🤔 |
Sorry, something went wrong.
|
I have production code that currently assumes parameters that default to |
Sorry, something went wrong.
|
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 |
Sorry, something went wrong.
Fidget-Spinner
left a comment
There was a problem hiding this comment.
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.
Sorry, something went wrong.
@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? |
Sorry, something went wrong.
Co-authored-by: Ken Jin <28750310+Fidget-Spinner@users.noreply.github.com>
|
@Tinche for classes Here are two examples, » ./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 🤔 |
Sorry, something went wrong.
Thanks for the info! |
Sorry, something went wrong.
|
Thanks for the great question! 👍 I've added this corner case to our tests in a separate PR: #30535 |
Sorry, something went wrong.
JelleZijlstra
left a comment
There was a problem hiding this comment.
I like this change, just a comment on the docs.
Sorry, something went wrong.
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. |
Sorry, something went wrong.
Co-authored-by: Jelle Zijlstra <jelle.zijlstra@gmail.com>
JelleZijlstra
left a comment
There was a problem hiding this comment.
@gvanrossum this is another one I'd like to merge.
@gpshead previously requested changes, but his request was addressed.
Sorry, something went wrong.
gvanrossum
left a comment
There was a problem hiding this comment.
IIUC, @gpshead mist still approve it.
Sorry, something went wrong.
Since 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)
|
Just re-found this issue because Hypothesis does indeed have a test for this 😂 Let's mention this change in https://docs.python.org/3.11/whatsnew/3.11.html, not just the docs for |
Sorry, something went wrong.
Useful related links:
https://bugs.python.org/issue46195