I dont think it's necessary to specify what class you want to override, as in the code you can test issubclass or isinstance, so you know if the class is a part of a mixin.
That's a very bad idea: explicit is better than implicit.
You can have name collision between different class type and even worst different signature for the same named method.
- register in the pool Mixin with a list of classes
- at the end of the load_module_graph:
- the pool is called to loop over all the classes
- test if it is a subclass of all the classes registered with the Mixin.
- add the Mixin to the pool class
I think it is a much simpler design and there is no need to have the Mixin applied before having loaded all the modules. And if there are cases where it is needed, then the module can just explicitly extend the needed classes with the Mixin.
Following cedric's suggestion, here is my design proposal.
Rather than patching the "last" class in the pool, I would like to inject the
new class right after the class it wants to replace.
Here is an example. Assuming we want to patch the ModelSQL class with a custom
OverridenModelSQL class on ir.ui.menu, the current mro (with an override in the
"my_module" module) looks like :
I quickly made up something that would work, but I don't think it is
clean enough to be integrated as is :
source = ModelSQL
target = OverridenModelSQL
def patch_model(klass):
if issubclass(klass, target) or not issubclass(klass, source):
return
for mro_class in reversed(klass.__mro__):
if mro_class == source:
continue
if source not in mro_class.__bases__:
continue
mro_class.__bases__ = tuple(
x if x != source else target
for x in mro_class.__bases__)
I tried calling the patch_model method on all classes in the pool at the
end of the pool.init method, and it behaved as expected. But I do not
know if manually updating the __bases__ attribute is acceptable.
Here we directly modify the base class inheritances, so it may be a
problem for multiple databases management. We could probably go around this
by creating another 'trytond.pool.ir.ui.menu' class and make it inherit
of both the original base class and the OverridenModelSQL class.
I do not see the point. This makes Tryton "Mixin" inheritance working differently than the usual inheritance.
Also modifying the __bases__ attribute is quite questioning (not sure it is allowed in every implementation).
Finally, I think it is good practice that Mixin should not depend on the place it has in the mro like that it prevents some side-effects.
> I do not see the point. This makes Tryton "Mixin" inheritance
> working differently than the usual inheritance.
I am not sure to understand what you mean, could you elaborate ?
> Also modifying the __bases__ attribute is quite questioning
> (not sure it is allowed in every implementation).
Agreed, it may need some investigation. But we could achieve the
same result by registering in two passes (add a register_meta
method on modules to "build" the base classes, then calling the
regular register).
> Finally, I think it is good practice that Mixin should not depend
> on the place it has in the mro like that it prevents some
> side-effects.
It simplifies the implementation of the current feature, but I
think it will make "fine-tuning" of Mixin classes a little
complicated. For instance, I think you said you wanted to be able
to override existing Mixin classes (do not remember exactly when,
so correct me if I'm wrong). But doing so will be difficult with
the current implementation.
For instance, if one wants to override the TaxLineMixin class,
the mro will be something like :
So the implementation of the TaxLineMixinOverride will have to
deal with all method overrides that may have taken place in
the models that extended the base TaxLineMixin class. IMO, the
"logical" Mixin extension should look like :
On 2017-03-08 15:01, Jean CAVALLO wrote:
> > I do not see the point. This makes Tryton "Mixin" inheritance
> > working differently than the usual inheritance.
>
> I am not sure to understand what you mean, could you elaborate ?
It is bad to have different behaviour.
> > Finally, I think it is good practice that Mixin should not depend
> > on the place it has in the mro like that it prevents some
> > side-effects.
>
> It simplifies the implementation of the current feature, but I
> think it will make "fine-tuning" of Mixin classes a little
> complicated.
I do not understand "fine-tuning".
> For instance, I think you said you wanted to be able
> to override existing Mixin classes (do not remember exactly when,
> so correct me if I'm wrong). But doing so will be difficult with
> the current implementation.
>
> For instance, if one wants to override the TaxLineMixin class,
> the mro will be something like :
>
> TaxLineMixinOverride
> account.invoice.line
> TaxLineMixin
I see no problem.
> So the implementation of the TaxLineMixinOverride will have to
> deal with all method overrides that may have taken place in
> the models that extended the base TaxLineMixin class.
I do not understand. You do not have to override all methods. It is
object oriented programming.
> IMO, the
> "logical" Mixin extension should look like :
>
> account.invoice.line
> TaxLineMixinOverride
> TaxLineMixin
I think we do not have the same logic.
And I do not see why one mro will be better than another except the one
that is consistent with the rest of the framework.
> On 2017-03-08 15:01, Jean CAVALLO wrote:
> > > I do not see the point. This makes Tryton "Mixin" inheritance
> > > working differently than the usual inheritance.
> >
> > I am not sure to understand what you mean, could you elaborate ?
>
> It is bad to have different behaviour.
I meant that I do not see what you mean by "This makes Tryton Mixin inheritance
working differently than the usual inheritance.
> > For instance, if one wants to override the TaxLineMixin class,
> > the mro will be something like :
> >
> > TaxLineMixinOverride
> > account.invoice.line
> > TaxLineMixin
>
> I see no problem.
Consider the following :
class Mixin:
def value(self):
return 10
class MixinOverride(Mixin):
def value(self):
return super(MixinOverride, self).value() + 1
This is classical bad design which introduces side effect and there is no *good* solution.
I think we must not complexity the feature for such case but keep it simple and predictive.
Also you proposal is something that is never done in Python standards.
There is no mechanism in Python for that so I do not think it could be the expected behaviour for any developer.