bpo-44662: Add ability to annotate types.Union by uriyyo · Pull Request #27214 · python/cpython

uriyyo · 2021-07-17T10:01:55Z

https://bugs.python.org/issue44662

uriyyo · 2021-07-17T10:02:12Z

@Fidget-Spinner Could you please review this PR?

Fidget-Spinner

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

Objects/unionobject.c

uriyyo · 2021-07-17T10:36:47Z

I created separate issue at issue tracker.

uriyyo · 2021-07-19T09:16:45Z

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

Fidget-Spinner · 2021-07-19T15:48:36Z

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 · 2021-07-19T16:13:09Z

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 · 2021-07-19T16:33:47Z

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

Fidget-Spinner · 2021-07-19T16:33:59Z

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

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

Objects/unionobject.c

Misc/NEWS.d/next/Core and Builtins/2021-07-17-13-41-58.bpo-44662.q22kWR.rst

…62.q22kWR.rst Co-authored-by: Ken Jin <28750310+Fidget-Spinner@users.noreply.github.com>

uriyyo · 2021-07-25T09:33:05Z

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

Fidget-Spinner · 2021-07-25T10:28:01Z

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 · 2021-07-25T23:59:34Z

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

JelleZijlstra · 2021-07-26T00:03:52Z

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.

Fidget-Spinner

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.

Objects/unionobject.c

Fidget-Spinner

Thanks

bedevere-bot · 2021-07-29T13:45:17Z

🤖 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 · 2021-07-29T19:44:50Z

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

miss-islington · 2021-07-29T19:46:13Z

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

bedevere-bot · 2021-07-29T19:46:20Z

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

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>

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

bedevere-bot added the awaiting review label Jul 17, 2021

the-knights-who-say-ni added the CLA signed label Jul 17, 2021

Fidget-Spinner reviewed Jul 17, 2021

View reviewed changes

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 added 3 commits July 19, 2021 11:21

bpo-44490: Add ability to serialise and annotate types.Union

5021ca7

Update news

43556fe

Update unionobject

eb52dc8

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 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 added 3 commits July 19, 2021 19:28

Remove Union serialisation code

2fcfafe

Fix _strip_annotations

65558f2

Remove whitespace

8a13208

Fidget-Spinner reviewed Jul 19, 2021

View reviewed changes

Objects/unionobject.c Outdated Show resolved Hide resolved

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

Merge remote-tracking branch 'upstream/main' into fix-issue-44490

f72e70f

Fidget-Spinner reviewed Jul 26, 2021

View reviewed changes

Objects/unionobject.c Outdated Show resolved Hide resolved

Objects/unionobject.c Outdated Show resolved Hide resolved

Remove __name__ and __qualname__ attrs

b931632

uriyyo requested a review from Fidget-Spinner July 26, 2021 09:36

Fidget-Spinner approved these changes Jul 26, 2021

View reviewed changes

bedevere-bot added awaiting core review and removed awaiting review labels Jul 26, 2021

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

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 removed the awaiting core review label Jul 29, 2021

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

bedevere-bot removed the needs backport to 3.10 only security fixes label Jul 29, 2021

Uh oh!

Comments

Conversation

uriyyo commented Jul 17, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

uriyyo commented Jul 17, 2021

Uh oh!

Fidget-Spinner left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

uriyyo commented Jul 17, 2021

Uh oh!

uriyyo commented Jul 19, 2021

Uh oh!

Fidget-Spinner commented Jul 19, 2021

Uh oh!

uriyyo commented Jul 19, 2021

Uh oh!

uriyyo commented Jul 19, 2021

Uh oh!

Fidget-Spinner commented Jul 19, 2021

Uh oh!

Fidget-Spinner left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

uriyyo commented Jul 25, 2021

Uh oh!

Fidget-Spinner commented Jul 25, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Fidget-Spinner commented Jul 25, 2021

Uh oh!

JelleZijlstra commented Jul 26, 2021

Uh oh!

Fidget-Spinner left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Fidget-Spinner left a comment

Choose a reason for hiding this comment

Uh oh!

bedevere-bot commented Jul 29, 2021

Uh oh!

bedevere-bot commented Jul 29, 2021

Uh oh!

miss-islington commented Jul 29, 2021

Uh oh!

bedevere-bot commented Jul 29, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

uriyyo commented Jul 17, 2021 •

edited

Loading

Fidget-Spinner left a comment •

edited

Loading

Fidget-Spinner commented Jul 25, 2021 •

edited

Loading