bpo-43923: Allow NamedTuple multiple inheritance by juliusgeo · Pull Request #31779

bpo-43923: Allow NamedTuple multiple inheritance by juliusgeo · Pull Request #31779 · python/cpython

Choose a reason for hiding this comment

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

This looks like it's still mostly a revert, which I don't think is the right approach since it ends up losing a bunch of past improvements. Try to make the minimal change necessary to make things work.

		def _make_nmtuple(name, types):
		msg = "NamedTuple('Name', [(f0, t0), (f1, t1), ...]); each t must be a type"
		types = [(n, _type_check(t, msg)) for n, t in types]
		nm_tpl = collections.namedtuple(name, [n for n, t in types])

Choose a reason for hiding this comment

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

collections.namedtuple now supports default= directly, so we should use that

		defaults=defaults, module=module)
		nm_tpl.__annotations__ = nm_tpl.__new__.__annotations__ = types
		def _make_nmtuple(name, types):
		msg = "NamedTuple('Name', [(f0, t0), (f1, t1), ...]); each t must be a type"

Choose a reason for hiding this comment

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

You're missing the improved error message from the existing code

		default_names=', '.join(defaults_dict.keys())))
		nm_tpl.__new__.__annotations__ = dict(types)
		nm_tpl.__new__.__defaults__ = tuple(defaults)
		nm_tpl._field_defaults = defaults_dict

Choose a reason for hiding this comment

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

No need to restore this old attribute

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

Note there's an alternative PR from Serhiy: #31781.

Note there's an alternative PR from Serhiy: #31781.

I still like Serhiy's more. In particular, I think it's best to keep NamedTuple a function, rather than a class. Given what inheriting from NamedTuple actually does (dynamically create a new tuple subclass), I think it makes much more sense for it to be a function.

We merged #92027 to allow generic namedtuples. I don't think we want more general multiple inheritance. Thanks for your PR though!