Skip to content

bpo-44490: Improve typing module compatibility with types.Union #27048

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.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 10 commits into from
Jul 17, 2021
Merged

bpo-44490: Improve typing module compatibility with types.Union #27048

merged 10 commits into from
Jul 17, 2021

Conversation

uriyyo
Copy link
Member

@uriyyo uriyyo commented Jul 6, 2021 •
edited by bedevere-bot
Loading

@uriyyo
Copy link
Member Author

uriyyo commented Jul 6, 2021

@Fidget-Spinner Could you please review this PR?

@Fidget-Spinner
Copy link
Member

@Fidget-Spinner Could you please review this PR?

The Python typing parts LGTM. But I need to do some revision for the C parts.

@Fidget-Spinner
Copy link
Member

Hmm I wonder if it would be better if you split out the pure Python changes (like the nested __parameters__ search in typing) from the C+Python changes into a separate PR.

My reasoning follows that the Python changes can easily be backported. The C changes not so much and will require much more deliberation. What do you think Guido? @gvanrossum

@gvanrossum
Copy link
Member

Agreed, this currently looks like two independent changes. See also the NEWS file.

@uriyyo
Copy link
Member Author

uriyyo commented Jul 7, 2021

@Fidget-Spinner @gvanrossum Thats makes sense. I will convert this PR into containing only typing fixes and will open another PR.

@uriyyo
Copy link
Member Author

uriyyo commented Jul 7, 2021

@Fidget-Spinner Could you please review this PR?

Fidget-Spinner
Fidget-Spinner reviewed Jul 8, 2021
View reviewed changes
Copy link
Member

@Fidget-Spinner Fidget-Spinner left a comment

Choose a reason for hiding this comment

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

LGTM. Some minor questions and comments below.

@uriyyo uriyyo changed the title bpo-44490: Fix various issues with types.Union bpo-44490: Improve typing module compatibility with types.Union Jul 8, 2021
@uriyyo uriyyo requested a review from Fidget-Spinner July 8, 2021 18:02
@uriyyo
Copy link
Member Author

uriyyo commented Jul 12, 2021

@Fidget-Spinner Could you please review this PR?

Fidget-Spinner
Fidget-Spinner approved these changes Jul 12, 2021
View reviewed changes
Copy link
Member

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

Choose a reason for hiding this comment

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

Yurii, sorry for the delay. Was a little busy. This LGTM. Thanks for submitting this PR.

BTW, it looks like the C changes to types.Union in the other PR won't make to 3.10 (we missed the beta 4 deadline). So we don't have to worry about backporting anymore.

@uriyyo
Copy link
Member Author

uriyyo commented Jul 12, 2021

Thanks for review, should we mention those changes at What's new section?

@Fidget-Spinner
Copy link
Member

Thanks for review, should we mention those changes at What's new section?

Yes I was just thinking about that ;). I recommend we mention in whatsnew for 3.11 that union now supports nested type parameters.

@uriyyo
Copy link
Member Author

uriyyo commented Jul 12, 2021

Great, should it be done at separate PR?

@Fidget-Spinner
Copy link
Member

Great, should it be done at separate PR?

Please do! I don't think the other C changes to fix typing.Annotated are whatsnew worthy. So you can start on it.

@uriyyo
Copy link
Member Author

uriyyo commented Jul 14, 2021

@gvanrossum Could you please review this PR?

gvanrossum
gvanrossum approved these changes Jul 17, 2021
View reviewed changes
Copy link
Member

@gvanrossum gvanrossum left a comment

Choose a reason for hiding this comment

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

Great work! I will merge.

@gvanrossum gvanrossum merged commit bf89ff9 into python:main Jul 17, 2021
@gvanrossum
Copy link
Member

@Fidget-Spinner Do you think this is worth a backport to 3.10? IIUC @serhiy-storchaka is backporting his types.Union improvements.

@Fidget-Spinner
Copy link
Member

I think it's worth a backport but we can't since the PR adding __parameters__ #26980 was never backported.

@gvanrossum
Copy link
Member

Never mind then. I guess that’s fine.

@Fidget-Spinner
Copy link
Member

Thanks to Serhiy's Mass Cleanup, we can now backport this.

@Fidget-Spinner Fidget-Spinner removed the request for review from ilevkivskyi July 18, 2021 02:27
@Fidget-Spinner Fidget-Spinner added the needs backport to 3.10 only security fixes label Jul 18, 2021
@miss-islington
Copy link
Contributor

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

@bedevere-bot
Copy link

GH-27220 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 18, 2021
miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Jul 18, 2021
@uriyyo @miss-islington
bpo-44490: Improve typing module compatibility with types.Union (pyth…
4ed37b5 
…onGH-27048)

(cherry picked from commit bf89ff9)

Co-authored-by: Yurii Karabas <[email protected]>
gvanrossum pushed a commit that referenced this pull request Jul 19, 2021
@Fidget-Spinner @uriyyo
bpo-44490: Improve typing module compatibility with types.Union (GH-2…
a272164 
…7048) (#27222)

(cherry picked from commit bf89ff9)

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

@gvanrossum gvanrossum gvanrossum approved these changes

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

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@uriyyo @Fidget-Spinner @gvanrossum @miss-islington @bedevere-bot @the-knights-who-say-ni