Skip to content

bpo-44731: Simplify the union type implementation #27318

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 1 commit into from
Jul 24, 2021
Merged

bpo-44731: Simplify the union type implementation #27318

merged 1 commit into from
Jul 24, 2021

Conversation

serhiy-storchaka
Copy link
Member

@serhiy-storchaka serhiy-storchaka commented Jul 24, 2021 •
edited by bedevere-bot
Loading

Remove direct support of typing types in the C code because they
are already supported by defining methods or and ror in
the Python code.

https://bugs.python.org/issue44731

@serhiy-storchaka
bpo-44731: Simplify the union type implementation
d54c7b4 
Remove direct support of typing types in the C code because they
are already supported by defining methods __or__ and __ror__ in
the Python code.
Fidget-Spinner
Fidget-Spinner approved these changes Jul 24, 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.

Thanks, LGTM. Are we backporting this to 3.10? If we don't then 3.10 -> 3.11 runtime may have a breaking change (returning typing.Union over types.Union for some types).

@serhiy-storchaka
Copy link
Member Author

It is not necessary to backport it to 3.10 because it does not fix bugs. But it makes the code much simpler and more reliable (the removed code uses tricks and was rewritten many times), and it is better to make potentially incompatible changes while there are not many users of this class.

But it is all up to the release manager. @pablogsal

@pablogsal
Copy link
Member

It is not necessary to backport it to 3.10 because it does not fix bugs. But it makes the code much simpler and more reliable (the removed code uses tricks and was rewritten many times), and it is better to make potentially incompatible changes while there are not many users of this class.

But it is all up to the release manager. @pablogsal

Thanks for checking @serhiy-storchaka !

I reviewed the code and I am comfortable backporting it as the union type is a new feature and this is not a lot of code and will help maintaining it and future backports.

But please, land it as soon as possible so people they test main can test it a bit before the RC 1.

@pablogsal pablogsal added the needs backport to 3.10 only security fixes label Jul 24, 2021
@pablogsal pablogsal merged commit 0828423 into python:main Jul 24, 2021
@miss-islington
Copy link
Contributor

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

@miss-islington
Copy link
Contributor

Sorry, @serhiy-storchaka and @pablogsal, I could not cleanly backport this to 3.10 due to a conflict.
Please backport using cherry_picker on command line.
cherry_picker 08284231275ac9cc60ae27eab2338805919d8881 3.10

@pablogsal
Copy link
Member

@serhiy-storchaka Can you do the backport?

@Fidget-Spinner
Copy link
Member

@pablogsal I'm currently doing the backport, but 3.11 and 3.10 are quite out of sync because GH-27244 wasn't backported.

@pablogsal
Copy link
Member

GH-27244 wasn't backported.

Sigh.... Let's backport GH-27244 then.

@pablogsal
Copy link
Member

@Fidget-Spinner You can try the backport now :)

@Fidget-Spinner Fidget-Spinner mentioned this pull request Jul 24, 2021
Merged
@Fidget-Spinner Fidget-Spinner added needs backport to 3.10 only security fixes and removed needs backport to 3.10 only security fixes labels Jul 24, 2021
@miss-islington
Copy link
Contributor

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

@miss-islington
Copy link
Contributor

Sorry @serhiy-storchaka and @pablogsal, I had trouble checking out the 3.10 backport branch.
Please backport using cherry_picker on command line.
cherry_picker 08284231275ac9cc60ae27eab2338805919d8881 3.10

@Fidget-Spinner
Copy link
Member

Well I tried... time to do it manually.

Fidget-Spinner pushed a commit to Fidget-Spinner/cpython that referenced this pull request Jul 24, 2021
@serhiy-storchaka @Fidget-Spinner
bpo-44731: Simplify the union type implementation (pythonGH-27318)
c173ab3 
Remove direct support of typing types in the C code because they are already supported by defining methods __or__ and __ror__ in the Python code.
@bedevere-bot
Copy link

GH-27334 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 24, 2021
pablogsal pushed a commit that referenced this pull request Jul 24, 2021
@Fidget-Spinner @serhiy-storchaka
bpo-44731: Simplify the union type implementation (GH-27318) (GH-27334)
ca5a4cf 
Remove direct support of typing types in the C code because they are already supported by defining methods __or__ and __ror__ in the Python code.

Co-authored-by: Serhiy Storchaka <storchaka@gmail.com>
@serhiy-storchaka serhiy-storchaka deleted the union-or branch July 24, 2021 18:35
@serhiy-storchaka
Copy link
Member Author

Thank you for backporting @Fidget-Spinner.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pablogsal pablogsal pablogsal approved these changes

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

@gvanrossum gvanrossum Awaiting requested review from gvanrossum

Assignees

@pablogsal pablogsal

Labels
skip news
Projects
None yet
Milestone
No milestone
Development

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