Multiple inheritance: order parameters by MRO

[hroskes]

I'm making this as a separate PR from https://github.com/biqqles/dataclassy/pull/27 because it's a more involved change. Instead of taking the defaults from the bases directly, they now follow the __mro__ of the class, as the test in 581332a957e33ce670138b00402d893736a2c3e9 illustrates.

This does change the signature of classes with multiple inheritance, as this diff shows. The new behavior is less intuitive in some ways, because you'd expect Alpha to come before Epsilon. On the other hand, when you think about it it makes more sense (at least for me), because Epsilon is a more distant parent and therefore comes first, just like how if you inherited from Beta, Alpha would be the more distant parent and would come first.

The new signature is also consistent with python's dataclasses.

[biqqles]

I deliberately didn't go with MRO but I honestly have no recollection of why. I'll try to remember in case it's important.

[biqqles]

I think the reason I didn't like MRO was that I found the parameter ordering confusing, and on top of that it actually makes the metaclass code more complex. Also, at this point changing the behaviour would be a breaking change so would need strong justification.

I'm not really bothered about the parameter ordering matching dataclasses since dataclassy's order is already deliberately different for normal inheritance.

I actually do use multiple inheritance somewhere in my own code so I'll take a look at that to see how the change would affect a real-world example before making a final decision.

[biqqles]

I don't think your branch works with slots=True:

File "/home/biqqles/.local/lib/python3.7/site-packages/dataclassy/dataclass.py", line 43, in __new__
    without_dc_meta = super().__new__(mcs, name, bases, dict_)
ValueError: 'base' in __slots__ conflicts with class variable

(base is a field on the class I linked to.)

I expect this just happens because the metaclass is initially naively instantiated before the logic to handle slots used for the "real thing". Is there no way to get MRO without doing this?

[hroskes]

Oh, I see. I haven't found a better way to get the mro - the top google result I found is this from 20 years ago, and I don't see anything more recent that's better. https://mail.python.org/pipermail/python-list/2002-December/142612.html I'll see if I can fix slots, unless you still specifically don't want to use mro.

[biqqles]

OK, I tried it with the class I linked above to compare the signatures for with a real-world example:

Without MRO

(nickname: str,
pos: flint.maps.pos,
_system: 'System',
archetype: str,
spin: Tuple[float, float, float],
reputation: str,
base: str,
ids_name: Union[int, NoneType] = None,
ids_info: Union[int, NoneType] = None,
rotate: flint.maps.rot = rot(x=0, y=0, z=0),
atmosphere_range: int = 0,
**kwargs)

With MRO

(nickname: str,
pos: flint.maps.pos,
_system: 'System',
archetype: str,
reputation: str,
base: str,
spin: Tuple[float, float, float],
ids_name: Union[int, NoneType] = None,
ids_info: Union[int, NoneType] = None,
rotate: flint.maps.rot = rot(x=0, y=0, z=0),
atmosphere_range: int = 0,
**kwargs)

As you can see the only difference is the placement of spin:

 archetype: str,
-spin: Tuple[float, float, float],
 reputation: str,
 base: str,
+spin: Tuple[float, float, float],
 ids_name: Union[int, NoneType] = None,

To be honest, neither of these have a huge advantage to my eyes. If anything, I like that without MRO has spin, which comes from the first base (Planet), first. While it would be difficult to mentally keep track of the parameter ordering of a class this complex, I do think it's a little more predictable to have multiple bases read left-to-right. You can in theory keep that rule in mind (along with the general heuristics of fields being in definition-order and fields with defaults being split from those without them. MRO on the other hand would make it virtually impossible to predict field ordering, I think.

Then there's the extra code to do this. Having to instantiate the class twice is slow and, in my opinion, quite ugly. I don't see that it provides a big advantage, especially as multiple inheritance is pretty rare. So my inclination is to not merge this change, sorry.

[hroskes]

No problem. TBH I'm not really attached to MRO for its own sake, more avoiding bugs like the ones we've been dealing with over the last few days which feel like they'd "automatically" be dealt with by using the MRO. Probably that's wishful thinking in any case. I'm chasing down another one at the moment, but I can close this PR.