GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — bpo-44662: Add ability to annotate types.Union by uriyyo · Pull Request #27214 · python/cpython

Conversation

@uriyyo

@uriyyo uriyyo commented Jul 17, 2021 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

https://bugs.python.org/issue44662

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@uriyyo
uriyyo requested a review from gvanrossum as a code owner July 17, 2021 10:01
@bedevere-bot bedevere-bot added the awaiting review label Jul 17, 2021
@the-knights-who-say-ni the-knights-who-say-ni added the CLA signed label Jul 17, 2021
@uriyyo

uriyyo commented Jul 17, 2021

Copy link
Copy Markdown
Member Author

@Fidget-Spinner Could you please review this PR?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
Fidget-Spinner
Fidget-Spinner reviewed Jul 17, 2021
View reviewed changes

@Fidget-Spinner Fidget-Spinner left a comment •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

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.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
uriyyo reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
Comment thread Objects/unionobject.c Resolved

Uh oh!

There was an error while loading. Please reload this page.
@uriyyo 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

uriyyo commented Jul 17, 2021

Copy link
Copy Markdown
Member Author

I created separate issue at issue tracker.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
uriyyo added 3 commits July 19, 2021 11:21
@uriyyo
bpo-44490: Add ability to serialise and annotate types.Union
5021ca7
@uriyyo
Update news
43556fe
@uriyyo
Update unionobject
eb52dc8
@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?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@uriyyo 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
@uriyyo
Remove Union serialisation code
2fcfafe
@uriyyo
Fix _strip_annotations
65558f2
@uriyyo
Remove whitespace
8a13208
@uriyyo

uriyyo commented Jul 19, 2021

Copy link
Copy Markdown
Member Author

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@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.
uriyyo reacted with laugh emoji uriyyo reacted with hooray emoji
All reactions
  • 😄 1 reaction
  • 🎉 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
Fidget-Spinner
Fidget-Spinner reviewed Jul 19, 2021
View reviewed changes

@Fidget-Spinner Fidget-Spinner left a comment

Copy link
Copy Markdown
Member

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.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Objects/unionobject.c Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Misc/NEWS.d/next/Core and Builtins/2021-07-17-13-41-58.bpo-44662.q22kWR.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.
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 <[email protected]>
@uriyyo
Add PyUnicode_Check to union_getattro
136f81c
@uriyyo
uriyyo requested a review from Fidget-Spinner July 19, 2021 17:04
@uriyyo
Merge remote-tracking branch 'upstream/main' into fix-issue-44490
f72e70f
@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?)
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@Fidget-Spinner

Fidget-Spinner commented Jul 25, 2021 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@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?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
Fidget-Spinner
Fidget-Spinner reviewed Jul 26, 2021
View reviewed changes

@Fidget-Spinner Fidget-Spinner left a comment

Copy link
Copy Markdown
Member

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Objects/unionobject.c Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Objects/unionobject.c Resolved

Uh oh!

There was an error while loading. Please reload this page.
@uriyyo
Remove __name__ and __qualname__ attrs
b931632
@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

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.

Thanks

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
fochoao reacted with thumbs up emoji uriyyo reacted with hooray emoji
All reactions
  • 👍 1 reaction
  • 🎉 1 reaction
@bedevere-bot bedevere-bot added awaiting core review and removed awaiting review labels Jul 26, 2021
@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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@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!
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-bot bedevere-bot removed the awaiting core review label Jul 29, 2021
@ambv ambv added the needs backport to 3.10 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.
🐍🍒⛏🤖
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-bot bedevere-bot removed the needs backport to 3.10 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 <[email protected]>
(cherry picked from commit 8182c83)

Co-authored-by: Yurii Karabas <[email protected]>
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 <[email protected]>
(cherry picked from commit 8182c83)

Co-authored-by: Yurii Karabas <[email protected]>
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

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

7 participants

@uriyyo @Fidget-Spinner @JelleZijlstra @bedevere-bot @miss-islington @ambv @the-knights-who-say-ni
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.