Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

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

Merged
JelleZijlstra merged 6 commits intopython:mainfromsobolevn:issue-46195
Mar 2, 2022

Conversation

@sobolevn
Copy link
Member

@sobolevnsobolevn commentedDec 30, 2021
edited by bedevere-bot
Loading

@merwok
Copy link
Member

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 thechanges 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 +now avoids 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

@sobolevnsobolevn changed the titlebpo-46195:get_type_hints does not addOptional anymorebpo-46195: Do not addOptional inget_type_hintsJan 2, 2022
@sobolevn
Copy link
MemberAuthor

@merwok thanks, it is way more readable now! Wording (and writing in general) in English is something I really try to improve.

merwok reacted with thumbs up emoji

@sobolevn
Copy link
MemberAuthor

@Zac-HD do we rely on this behavior inhypothesis? 🤔

@Zac-HD
Copy link
Contributor

@Zac-HD do we rely on this behavior inhypothesis? 🤔

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
Copy link
Contributor

Will this be applied in Python 3.11, or is it intended to be back-ported to previous versions?

@sobolevn
Copy link
MemberAuthor

In my opinion this should be 3.11+ only 🤔
But, there might be other opinions of course.

AlexWaygood, pbryan, and gpshead reacted with thumbs up emoji

@sobolevnsobolevn added the type-featureA feature request or enhancement labelJan 9, 2022
@pbryan
Copy link
Contributor

I have production code that currently assumes parameters that default toNone are implicitlyOptional; the required changes to accommodate this are trivial on my part, and I can certainly plan within the 3.11 time horizon.

sobolevn and gpshead reacted with thumbs up emoji

@bedevere-bot
Copy link

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 phraseI 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.

Copy link
Member

@Fidget-SpinnerFidget-Spinner left a 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.

@Tinche
Copy link
Contributor

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.

@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?

Fidget-Spinner reacted with thumbs up emoji

Co-authored-by: Ken Jin <28750310+Fidget-Spinner@users.noreply.github.com>
@sobolevn
Copy link
MemberAuthor

sobolevn commentedJan 11, 2022
edited
Loading

@Tinche for classesOptional was never added implictly (which is another point why we should fix this inconsistency).

Here are two examples,main and3.9:

» ./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
Copy link
Contributor

@Tinche for classesOptional was never added implictly (which is another point why we should fix this inconsistency).

Here are two examples,main and3.9:

» ./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 🤔

Thanks for the info!

@sobolevn
Copy link
MemberAuthor

Thanks for the great question! 👍

I've added this corner case to our tests in a separate PR:#30535

Copy link
Member

@JelleZijlstraJelleZijlstra left a 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.

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 toNone was set. Now the annotation is returned unchanged."

Copy link
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Great addition!

@samuelcolvin
Copy link
Contributor

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 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.

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.

sobolevn reacted with hooray emoji

@sobolevnsobolevn reopened thisFeb 3, 2022
@JelleZijlstraJelleZijlstra self-assigned thisFeb 17, 2022
Co-authored-by: Jelle Zijlstra <jelle.zijlstra@gmail.com>
Copy link
Member

@JelleZijlstraJelleZijlstra left a 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.

Copy link
Member

@gvanrossumgvanrossum left a 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.

@JelleZijlstraJelleZijlstra merged commit20a1c8e intopython:mainMar 2, 2022
tk0miya added a commit to tk0miya/sphinx that referenced this pull requestMar 2, 2022
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
Copy link
Contributor

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 fortyping.get_type_hints().

Fidget-Spinner reacted with thumbs up emoji

Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@AlexWaygoodAlexWaygoodAlexWaygood left review comments

@gpsheadgpsheadgpshead approved these changes

@JelleZijlstraJelleZijlstraJelleZijlstra approved these changes

@gvanrossumgvanrossumgvanrossum approved these changes

@Fidget-SpinnerFidget-SpinnerAwaiting requested review from Fidget-Spinner

Assignees

@JelleZijlstraJelleZijlstra

Labels

type-featureA feature request or enhancement

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

13 participants

@sobolevn@merwok@Zac-HD@pbryan@bedevere-bot@Tinche@samuelcolvin@gpshead@JelleZijlstra@gvanrossum@Fidget-Spinner@AlexWaygood@the-knights-who-say-ni

[8]ページ先頭

©2009-2025 Movatter.jp