Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork33.7k
bpo-46195: Do not addOptional inget_type_hints#30304
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
merwok commentedJan 2, 2022
A note about commit messages / PR titles:
So for this PR (if I understand current title correctly) it could be:
|
get_type_hints does not addOptional anymoreOptional inget_type_hintssobolevn commentedJan 2, 2022
@merwok thanks, it is way more readable now! Wording (and writing in general) in English is something I really try to improve. |
sobolevn commentedJan 9, 2022
@Zac-HD do we rely on this behavior in |
Zac-HD commentedJan 9, 2022
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. |
pbryan commentedJan 9, 2022
Will this be applied in Python 3.11, or is it intended to be back-ported to previous versions? |
sobolevn commentedJan 9, 2022
In my opinion this should be 3.11+ only 🤔 |
pbryan commentedJan 9, 2022
I have production code that currently assumes parameters that default to |
Uh oh!
There was an error while loading.Please reload this page.
bedevere-bot commentedJan 9, 2022
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 |
Fidget-Spinner left a comment
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.
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 useget_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 withOptional if it sees a defaultNone. This PR proposes to cease that behavior.
Misc/NEWS.d/next/Library/2021-12-30-21-38-51.bpo-46195.jFKGq_.rst OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
Tinche commentedJan 11, 2022
@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? |
Co-authored-by: Ken Jin <28750310+Fidget-Spinner@users.noreply.github.com>
sobolevn commentedJan 11, 2022 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
@Tinche for classes Here are two examples, » ./python.exePython3.11.0a3+ (heads/main:e13cdca0f5,Jan112022,12:43:49) [Clang11.0.0 (clang-1100.0.33.16)]ondarwinType"help","copyright","credits"or"license"formoreinformation.>>>importtyping>>>classA:...x:int=None...>>>typing.get_type_hints(A){'x':<class'int'>} »pythonPython3.9.9 (main,Dec212021,11:35:28) [Clang11.0.0 (clang-1100.0.33.16)]ondarwinType"help","copyright","credits"or"license"formoreinformation.>>>importtyping>>>classA:...x:int=None...>>>typing.get_type_hints(A){'x':<class'int'>} However, I can't find a test case for this 🤔 |
Tinche commentedJan 11, 2022
Thanks for the info! |
sobolevn commentedJan 11, 2022
Thanks for the great question! 👍 I've added this corner case to our tests in a separate PR:#30535 |
JelleZijlstra left a comment
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 like this change, just a comment on the docs.
Doc/library/typing.rst Outdated
| ``Optional`` annotation is not added implicitly anymore | ||
| when ``None`` default is used for a function argument. |
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 like the wording in the removed docs better. Suggestion:
"Previously,Optional[t] was added for function and method annotations if a default value equal toNone was set. Now the annotation is returned unchanged."
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.
Great addition!
samuelcolvin commentedJan 26, 2022
Thanks@Fidget-Spinner for asking my opinion, really useful to get a heads up about things like this. I don'tthink 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. |
Misc/NEWS.d/next/Library/2021-12-30-21-38-51.bpo-46195.jFKGq_.rst OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
Co-authored-by: Jelle Zijlstra <jelle.zijlstra@gmail.com>
JelleZijlstra left a comment
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.
@gvanrossum this is another one I'd like to merge.
@gpshead previously requested changes, but his request was addressed.
gvanrossum left a comment
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.
IIUC,@gpshead mist still approve it.
Since python-3.11, `typing.get_type_hints()` will not add Optional[t] totype annotations even if a default value for function argument is None.refs:python/cpython#30304 (bpo-46195)
Zac-HD commentedMar 14, 2022
Just re-found this issue because Hypothesis does indeed have a test for this 😂 Let's mention this change inhttps://docs.python.org/3.11/whatsnew/3.11.html, not just the docs for |
Uh oh!
There was an error while loading.Please reload this page.
Useful related links:
https://bugs.python.org/issue46195