Skip to content
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.

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

Extendable NamedTuples #437

Closed

Conversation

ilevkivskyi
Copy link
Member

Fixes #427

There are three notes:

First, NamedTuple must be explicitly mentioned in the bases to get the desired behavior. This is necessary since typing.NamedTuple returns a plain collections.namedtuple, so that one needs NamedTuple to invoke NamedTupleMeta:

class A(NamedTuple):
    x: int
    y: int
class B(A, NamedTuple):
    z: int

Second, in the case of multiple inheritance fields are collected in reversed order, since this is similar to how MRO works, so that this:

class Point(NamedTuple):
    x: int
    y: int
class Label(NamedTuple):
    label: str
class LabeledPoint(Label, Point, NamedTuple):
    pass

is equivalent to

class LabeledPoint(NamedTuple):
    x: int
    y: int
    label: str  # this comes last

Finally, user defined methods are not inherited for sub-namedtuples (since the resulting collections.namedtuple has only single base -- tuple). This could be patched by manually recalculating the MRO, but then typing.NamedTuple becomes overly complicated.

In general, I propose this to be the last feature of typing.NamedTuple, named tuples are designed to be simple compact classes. I propose to direct all future feature requests to dataclasses.

@gvanrossum
Copy link
Member

Finally, user defined methods are not inherited for sub-namedtuples (since the resulting collections.namedtuple has only single base -- tuple). This could be patched by manually recalculating the MRO, but then typing.NamedTuple becomes overly complicated.

This sounds disturbing -- is this only a problem when you're mixing namedtuple and NamedTuple? Then it's fine.

@ilevkivskyi
Copy link
Member Author

This sounds disturbing -- is this only a problem when you're mixing namedtuple and NamedTuple?

NamedTupleMeta always creates named tuple classes just by calling collections.namedtuple and then manually populating the namespace of the freshly created class (which is just a simple subclass of tuple) with the definitions (methods). There is no other way we can put user-defined methods into class returned by collections.namedtuple (it is created using exec).

@JukkaL
Copy link
Contributor

JukkaL commented Jun 9, 2017

If methods can't be inherited, I'd rather reject inheritance from named tuple with user methods than silently drop them.

What about isinstance? Is a derived named tuple a subclass/subtype of the base named tuple or not? If not, the use of class syntax is kind of confusing, as subclassing implies subtyping elsewhere.

@gvanrossum
Copy link
Member

This all makes me worry about the entire feature and regret I even proposed it (and I haven't even peeked at the code yet).

At the very least the M.I. feels very wrong, since moving the fields of Label means that the index position of the label field cannot be trusted. (Imagine some code written to expect a Label and using a[0] instead of a.label, and being passed in a LabeledPoint.)

@ilevkivskyi
Copy link
Member Author

ilevkivskyi commented Jun 9, 2017

@JukkaL

If methods can't be inherited, I'd rather reject inheritance from named tuple with user methods than silently drop them.

Not that it is impossible, it is possible by manually reconstructing MRO, but I think we should not do this. If Guido is fine with raising an exception, then I will do this.

What about isinstance?

It returns False. The only base of all named tuples is tuple.

If not, the use of class syntax is kind of confusing

There is an alternative option:

class C(NamedTuple, extend_fields_from=[A, B]):  # kwarg name can be discussed :-)
    ...

Then it will be easy to explain why we drop methods from A and B.

@gvanrossum

This all makes me worry about the entire feature and regret I even proposed it

I have seen this request few times elsewhere (on Stackoverflow?) so that we may want to implement it. But we should probably only support basic/simple extending.

@ilevkivskyi
Copy link
Member Author

After thinking a bit I am inclined to only support something as simple as:

class C(NamedTuple, extend_fields_from=B):
    ...

with only one "base" named tuple. If someone will ask for something more complex, then one should use dataclasses instead.

@gvanrossum
Copy link
Member

Hm, ok, the keyword arg makes it clear that this is not inheritance per se, but it's a rarely used pattern (many experienced Python users do a double-take when they see it for the first time).

Also, while it makes sense that there is no actual inheritance (neither in the static type system nor at runtime), because the created types inherits from collections.namedtuple, people will still be confused, because they can't have a function that accepts an A and pass it a B. (They can, but mypy will reject it, and isinstance(arg, A) will be false.)

At this point I think we need to either reject the feature proposal or put it off until Python 3.7. It may also make sense to continue the design discussion in the original issue rather than in this PR.

@ilevkivskyi
Copy link
Member Author

OK, let us close this for now. I will make an alternative proposal in the issue soon.

@ilevkivskyi ilevkivskyi closed this Jun 9, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants