← 返回首页
bpo-44662: Add ability to annotate types.Union by uriyyo · Pull Request #27214 · python/cpython · GitHub
Skip to content

Navigation Menu

Toggle navigation
Sign in
Appearance settings
Search or jump to...

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

Provide feedback

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

Include my email address so I can be contacted
Cancel Submit feedback

Saved searches

Use saved searches to filter your results more quickly

Cancel Create saved search
Appearance settings
Resetting focus

bpo-44662: Add ability to annotate types.Union#27214

Merged
ambv merged 10 commits into
python:mainfrom
uriyyo:fix-issue-44490
Jul 29, 2021
Merged

bpo-44662: Add ability to annotate types.Union#27214
ambv merged 10 commits into
python:mainfrom
uriyyo:fix-issue-44490

Conversation

uriyyo commented Jul 17, 2021 •
edited
Loading

Copy link
Copy Markdown
Member

uriyyo requested a review from gvanrossum as a code owner July 17, 2021 10:01

uriyyo commented Jul 17, 2021

Copy link
Copy Markdown
Member Author

@Fidget-Spinner Could you please review this PR?

Fidget-Spinner reviewed Jul 17, 2021
View reviewed changes

Fidget-Spinner left a comment •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality Hide comment

Please wait until Serhiy merges some of his union refactoring changes. He's already prepared them, just not yet made PRs for review and merge. This will also take some time for me to review as I need to revise on tp_getattro and friends. Sorry (I'm busy with other things at the moment).

Also the current issue is getting too big. Can you please create a separate issue for this on bpo then tie this PR to that? We are getting pretty far from my original issue on the bpo ;).

Comment thread Objects/unionobject.c Outdated Show resolved Hide resolved
uriyyo changed the title bpo-44490: Add ability to serialise and annotate types.Union bpo-44662: Add ability to serialise and annotate types.Union Jul 17, 2021

uriyyo commented Jul 17, 2021

Copy link
Copy Markdown
Member Author

I created separate issue at issue tracker.

uriyyo force-pushed the fix-issue-44490 branch from 97a1898 to eb52dc8 Compare July 19, 2021 09:04
uriyyo requested a review from Fidget-Spinner July 19, 2021 09:15

uriyyo commented Jul 19, 2021

Copy link
Copy Markdown
Member Author

@Fidget-Spinner I have merged Serhiy changes. Could you please review this PR?

Fidget-Spinner commented Jul 19, 2021

Copy link
Copy Markdown
Member

Can you please split the PR into two? It seems it's fixing pickling and typing.Annotated compatibility at the same time. Please create another issue for the annotated problem.

uriyyo commented Jul 19, 2021

Copy link
Copy Markdown
Member Author

Sorry about that, I thought that it will be a great idea to fix several issues in scope of one PR.

I will divide it at 2 separate PR.

uriyyo changed the title bpo-44662: Add ability to serialise and annotate types.Union bpo-44662: Add ability to annotate types.Union Jul 19, 2021

uriyyo commented Jul 19, 2021

Copy link
Copy Markdown
Member Author

@Fidget-Spinner This PR contains only typing.Annotated compatibility fix.

Fidget-Spinner commented Jul 19, 2021

Copy link
Copy Markdown
Member

Sorry about that, I thought that it will be a great idea to fix several issues in scope of one PR.

I will divide it at 2 separate PR.

No worries. Thanks for separating them. It's much clearer to me now.

Fidget-Spinner reviewed Jul 19, 2021
View reviewed changes

Fidget-Spinner left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality Hide comment

Looks good, I'll nosy a few others in too in case I missed something.

Comment thread Objects/unionobject.c Outdated Show resolved Hide resolved
Comment thread Misc/NEWS.d/next/Core and Builtins/2021-07-17-13-41-58.bpo-44662.q22kWR.rst Outdated Show resolved Hide resolved
uriyyo and others added 2 commits July 19, 2021 19:55
Update Misc/NEWS.d/next/Core and Builtins/2021-07-17-13-41-58.bpo-446… …
36db2f5
…62.q22kWR.rst Co-authored-by: Ken Jin <28750310+Fidget-Spinner@users.noreply.github.com>
Add PyUnicode_Check to union_getattro
136f81c
uriyyo requested a review from Fidget-Spinner July 19, 2021 17:04

uriyyo commented Jul 25, 2021

Copy link
Copy Markdown
Member Author

@Fidget-Spinner This PR is ready to be review. Could you please do it?)

Fidget-Spinner commented Jul 25, 2021 •
edited
Loading

Copy link
Copy Markdown
Member

I'm thinking about whether we should just fix typing._GenericAlias to look for type(o).__module__ if o.__module__ doesn't exist. It seems easier than passing lookups to the parent type in types.Union.

types.GenericAlias is a little special because it's supposed to be nearly completely transparent and pass everything to __origin__. But no other builtin type behaves like this. Eg. {}.__module__ doesn't have __name__ or __qualname__ etc. Then again, types.Union isn't like the other builtin types is it?

I'll ping Łukasz when it's monday for him. Or maybe Jelle can give his opinion too.

Off topic: I'm trying not to review stuff on sundays so that I have more time to work on other things :). So in the future I won't be able to respond very quickly to review requests on sunday.

Fidget-Spinner commented Jul 25, 2021

Copy link
Copy Markdown
Member

@ambv or @JelleZijlstra what do y'all think? Should we fix the typing module's Annotated or fix types.Union?

JelleZijlstra commented Jul 26, 2021

Copy link
Copy Markdown
Member

I haven't looked deeply but in general I'd prefer to make Annotated do as little runtime checking as possible, so we don't have to jump through hoops to make things like Union work with it.

13 hidden items Load more…
Fidget-Spinner reviewed Jul 26, 2021
View reviewed changes

Fidget-Spinner left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality Hide comment

I haven't looked deeply but in general I'd prefer to make Annotated do as little runtime checking as possible, so we don't have to jump through hoops to make things like Union work with it.

Thanks for your input Jelle :).

LGTM mostly. I'm convinced we should add __module__ to Union now. Thanks for your efforts Yurii.

Comment thread Objects/unionobject.c Outdated Show resolved Hide resolved
Comment thread Objects/unionobject.c Outdated Show resolved Hide resolved
uriyyo requested a review from Fidget-Spinner July 26, 2021 09:36
Fidget-Spinner approved these changes Jul 26, 2021
View reviewed changes

Fidget-Spinner left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality Hide comment

Thanks

ambv added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Jul 29, 2021

bedevere-bot commented Jul 29, 2021

Copy link
Copy Markdown

🤖 New build scheduled with the buildbot fleet by @ambv for commit b931632 🤖

If you want to schedule another build, you need to add the ":hammer: test-with-buildbots" label again.

bedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Jul 29, 2021
ambv merged commit 8182c83 into python:main Jul 29, 2021

bedevere-bot commented Jul 29, 2021

Copy link
Copy Markdown

@ambv: Please replace # with GH- in the commit message next time. Thanks!

ambv added the needs backport to 3.10 only security fixes label Jul 29, 2021

miss-islington commented Jul 29, 2021

Copy link
Copy Markdown
Contributor

Thanks @uriyyo for the PR, and @ambv for merging it 🌮🎉.. I'm working now to backport this PR to: 3.10.
🐍🍒⛏🤖

bedevere-bot commented Jul 29, 2021

Copy link
Copy Markdown

GH-27461 is a backport of this pull request to the 3.10 branch.

bedevere-bot removed the needs backport to 3.10 only security fixes label Jul 29, 2021
miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Jul 29, 2021
bpo-44662: Add ability to annotate types.Union (pythonGH-27214) …
fb62f18
Co-authored-by: Ken Jin <28750310+Fidget-Spinner@users.noreply.github.com> (cherry picked from commit 8182c83) Co-authored-by: Yurii Karabas <1998uriyyo@gmail.com>
ambv pushed a commit that referenced this pull request Jul 30, 2021
bpo-44662: Add ability to annotate types.Union (GH-27214) (GH-27461) …
4729976
Co-authored-by: Ken Jin <28750310+Fidget-Spinner@users.noreply.github.com> (cherry picked from commit 8182c83) Co-authored-by: Yurii Karabas <1998uriyyo@gmail.com>
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
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

Fidget-Spinner Fidget-Spinner approved these changes
gvanrossum Awaiting requested review from gvanrossum

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

Footer

© 2026 GitHub, Inc.