Skip to content

gh-116241: Add support of multiple inheritance with typing.NamedTuple #31781

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
Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

gh-116241: Add support of multiple inheritance with typing.NamedTuple #31781

wants to merge 6 commits into from
+43 −5

Conversation

serhiy-storchaka
Copy link
Member

@serhiy-storchaka serhiy-storchaka commented Mar 9, 2022 •
edited by bedevere-app bot
Loading

@serhiy-storchaka serhiy-storchaka added the type-feature A feature request or enhancement label Mar 9, 2022
@JelleZijlstra JelleZijlstra mentioned this pull request Mar 10, 2022
Closed
@FHTMitchell FHTMitchell mannequin mentioned this pull request Apr 28, 2022
Closed
@serhiy-storchaka serhiy-storchaka force-pushed the typing-namedtuple-multiple-inheritance branch from 637bdd3 to 64f0c5f Compare April 28, 2022 16:48
@serhiy-storchaka serhiy-storchaka marked this pull request as ready for review April 28, 2022 17:18
JelleZijlstra
JelleZijlstra reviewed Apr 29, 2022
View reviewed changes
Copy link
Member

@JelleZijlstra JelleZijlstra left a comment

Choose a reason for hiding this comment

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

I think this goes too far because it's not realistic to expect type checkers to support arbitrary base classes on NamedTuple. Also, there haven't been any user requests that I can see for multiple inheritance with anything other than Generic. So I'd prefer to merge the other PR that allows multiple inheritance with Generic only.

@AlexWaygood
Copy link
Member

I think this goes too far because it's not realistic to expect type checkers to support arbitrary base classes on NamedTuple. Also, there haven't been any user requests that I can see for multiple inheritance with anything other than Generic. So I'd prefer to merge the other PR that allows multiple inheritance with Generic only.

I also think it makes sense to only allow multiple inheritance with Generic, for now. It's a smaller change; and, there should be opportunity to rethink it in the future if it turns out that people really need arbitrary multiple inheritance for whatever reason.

@gvanrossum
Copy link
Member

So, let's just close this?

@serhiy-storchaka serhiy-storchaka mentioned this pull request Mar 2, 2024
Open
@serhiy-storchaka serhiy-storchaka changed the title bpo-43923: Add support of multiple inheritance with typing.NamedTuple gh-116241: Add support of multiple inheritance with typing.NamedTuple Mar 2, 2024
@serhiy-storchaka serhiy-storchaka force-pushed the typing-namedtuple-multiple-inheritance branch from 5077333 to d4bc711 Compare March 2, 2024 16:09
Jason-Y-Z
Jason-Y-Z approved these changes Mar 4, 2024
View reviewed changes
Copy link
Contributor

@Jason-Y-Z Jason-Y-Z left a comment

Choose a reason for hiding this comment

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

people really need arbitrary multiple inheritance for whatever reason

Thanks for reopening! This looks like the case now indeed

CoolCat467
CoolCat467 approved these changes Oct 9, 2024
View reviewed changes
Copy link

@CoolCat467 CoolCat467 left a comment

Choose a reason for hiding this comment

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

Looks great! I ended up doing nearly exactly the same thing in some code of my own to get around this limitation.

@Fidget-Spinner Fidget-Spinner removed their request for review October 9, 2024 14:36
AlexWaygood
AlexWaygood reviewed Oct 9, 2024
View reviewed changes
gvanrossum
gvanrossum reviewed Oct 9, 2024
View reviewed changes
@serhiy-storchaka
Copy link
Member Author

Thank you @AlexWaygood and @gvanrossum. Updated to 3.14 and applied the suggestions.

AlexWaygood
AlexWaygood approved these changes Oct 9, 2024
View reviewed changes
Copy link
Member

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

Given that we've had multiple requests for this, I'm okay with landing it. But please get sign-offs from @gvanrossum and @JelleZijlstra before merging.

gvanrossum
gvanrossum approved these changes Oct 9, 2024
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.

Let's do it! (I approve of the idea, I haven't carefully reviewed the code.)

@JelleZijlstra JelleZijlstra self-requested a review October 9, 2024 18:41
@JelleZijlstra
Copy link
Member

I'm OK with this given the discussion on the issue; will take another look at the code soon.

@hugovk
Copy link
Member

hugovk commented Apr 23, 2025

Reminder for this: the 3.14 beta freeze is in two weeks.

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

@Jason-Y-Z Jason-Y-Z Jason-Y-Z approved these changes

@CoolCat467 CoolCat467 CoolCat467 approved these changes

@AlexWaygood AlexWaygood AlexWaygood approved these changes

@JelleZijlstra JelleZijlstra Awaiting requested review from JelleZijlstra JelleZijlstra is a code owner

Assignees
No one assigned
Labels
awaiting merge topic-typing type-feature A feature request or enhancement
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
9 participants
@serhiy-storchaka @AlexWaygood @gvanrossum @JelleZijlstra @hugovk @Jason-Y-Z @CoolCat467 @the-knights-who-say-ni @bedevere-bot