◐ Shell
reader mode source ↗
Skip to content

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

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

bpo-44490: Improve typing module compatibility with types.Union#27048
gvanrossum merged 10 commits into
python:mainfrom
uriyyo:fix-issue-44490

Conversation

@uriyyo

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

Copy link
Copy Markdown
Member

@uriyyo

uriyyo commented Jul 6, 2021

Copy link
Copy Markdown
Member Author

@Fidget-Spinner Could you please review this PR?

@Fidget-Spinner

Fidget-Spinner commented Jul 6, 2021

Copy link
Copy Markdown
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

Fidget-Spinner commented Jul 7, 2021

Copy link
Copy Markdown
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

gvanrossum commented Jul 7, 2021

Copy link
Copy Markdown
Member

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

@uriyyo

uriyyo commented Jul 7, 2021

Copy link
Copy Markdown
Member Author

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

@uriyyo

uriyyo commented Jul 7, 2021

Copy link
Copy Markdown
Member Author

@Fidget-Spinner Could you please review this PR?

Fidget-Spinner
Fidget-Spinner reviewed Jul 8, 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

LGTM. Some minor questions and comments below.

@uriyyo @Fidget-Spinner
Update Misc/NEWS.d/next/Core and Builtins/2021-07-06-22-22-15.bpo-444… …
140a41a 
…90.BJxPbZ.rst

Co-authored-by: Ken Jin <28750310+Fidget-Spinner@users.noreply.github.com>
@uriyyo uriyyo changed the title bpo-44490: Fix various issues with types.Union Jul 8, 2021
uriyyo and others added 3 commits July 8, 2021 20:50
@uriyyo @Fidget-Spinner
Update Lib/test/test_typing.py …
6826d72 
Co-authored-by: Ken Jin <28750310+Fidget-Spinner@users.noreply.github.com>
@uriyyo
Update typing tests
8b36d36
@uriyyo
Update tests
4081113
@uriyyo uriyyo requested a review from Fidget-Spinner July 8, 2021 18:02
@uriyyo

uriyyo commented Jul 12, 2021

Copy link
Copy Markdown
Member Author

@Fidget-Spinner Could you please review this PR?

Fidget-Spinner
Fidget-Spinner approved these changes Jul 12, 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

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

uriyyo commented Jul 12, 2021

Copy link
Copy Markdown
Member Author

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

@Fidget-Spinner

Fidget-Spinner commented Jul 12, 2021

Copy link
Copy Markdown
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

uriyyo commented Jul 12, 2021

Copy link
Copy Markdown
Member Author

Great, should it be done at separate PR?

@Fidget-Spinner

Fidget-Spinner commented Jul 12, 2021

Copy link
Copy Markdown
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

uriyyo commented Jul 14, 2021

Copy link
Copy Markdown
Member Author

@gvanrossum Could you please review this PR?

gvanrossum
gvanrossum approved these changes Jul 17, 2021
View reviewed changes

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

Great work! I will merge.

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

gvanrossum commented Jul 17, 2021

Copy link
Copy Markdown
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

Fidget-Spinner commented Jul 17, 2021

Copy link
Copy Markdown
Member

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

@gvanrossum

gvanrossum commented Jul 17, 2021

Copy link
Copy Markdown
Member

Never mind then. I guess that’s fine.

@Fidget-Spinner

Fidget-Spinner commented Jul 18, 2021

Copy link
Copy Markdown
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

miss-islington commented Jul 18, 2021

Copy link
Copy Markdown
Contributor

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

@bedevere-bot

bedevere-bot commented Jul 18, 2021

Copy link
Copy Markdown

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 <1998uriyyo@gmail.com>
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 <1998uriyyo@gmail.com>
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