◐ Shell
reader mode source ↗
Skip to content

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

@uriyyo uriyyo commented Jul 17, 2021 •
edited
Loading

Copy link
Copy Markdown
Member

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

uriyyo commented Jul 17, 2021

Copy link
Copy Markdown
Member Author

@Fidget-Spinner Could you please review this PR?

Fidget-Spinner
Fidget-Spinner reviewed Jul 17, 2021
View reviewed changes

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

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

@uriyyo uriyyo changed the title bpo-44490: Add ability to serialise and annotate types.Union Jul 17, 2021
@uriyyo

uriyyo commented Jul 17, 2021

Copy link
Copy Markdown
Member Author

I created separate issue at issue tracker.

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

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

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

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 uriyyo changed the title bpo-44662: Add ability to serialise and annotate types.Union Jul 19, 2021
@uriyyo

uriyyo commented Jul 19, 2021

Copy link
Copy Markdown
Member Author

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

@Fidget-Spinner

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
Fidget-Spinner reviewed Jul 19, 2021
View reviewed changes

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

Hide comment

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

uriyyo and others added 2 commits July 19, 2021 19:55
@uriyyo @Fidget-Spinner
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>
@uriyyo
Add PyUnicode_Check to union_getattro
136f81c
@uriyyo uriyyo requested a review from Fidget-Spinner July 19, 2021 17:04
@uriyyo

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

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

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

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
Fidget-Spinner reviewed Jul 26, 2021
View reviewed changes

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

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.

@uriyyo uriyyo requested a review from Fidget-Spinner July 26, 2021 09:36
Fidget-Spinner
Fidget-Spinner approved these changes Jul 26, 2021
View reviewed changes

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

Hide comment

Thanks

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

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

bedevere-bot commented Jul 29, 2021

Copy link
Copy Markdown

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

@ambv ambv added the needs backport to 3.10 only security fixes label Jul 29, 2021
@miss-islington

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

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 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
@uriyyo @miss-islington
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
@miss-islington @uriyyo
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Fidget-Spinner Fidget-Spinner Fidget-Spinner approved these changes

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

@uriyyo @Fidget-Spinner @JelleZijlstra @bedevere-bot @miss-islington @ambv @the-knights-who-say-ni