8

I try to figure out what is the best practice in Python inheritance principles, when there is a 'bad idea' to change method signature in a child.

Let's suppose we have some base class BaseClient with already implemented create method (and some abstract ones) that fits good for almost all 'descendants' except one:

 class BaseClient(object):

     def __init__(self, connection=None):
         pass

     def create(self, entity_id, data=None):
         pass

 class ClientA(BaseClient):
     pass

 class ClientB(BaseClient):
     pass

The only class ClientC needs another implementation of create method with a little bit another method signature

 class ClientC(BaseClient):
     ....
     def create(self, data):
         pass

So the question is how to make this in a more 'pythonic' way, taking into account best python practice? Of course we can use *args, **kwargs and other **kwargs-like approaches in parent (child) method, but I'm afraid it makes my code less readable (self-documented).

martineau
  • 119,623
  • 25
  • 170
  • 301
machin
  • 440
  • 8
  • 21
  • 5
    Substitution principle: It is usually not good design to have a subclass that cannot do what base class can - `x.create(entity_id)` works for `BaseClient` so it should work for its subclasses. – zch Aug 05 '17 at 14:49
  • Why does ClientC need another implementation of create? – Gribouillis Aug 05 '17 at 14:52
  • 6
    What zch said: a subtype can add new args to the method signature, but it shouldn't remove them because that violates the [Liskov substitution principle](https://en.wikipedia.org/wiki/Liskov_substitution_principle). – PM 2Ring Aug 05 '17 at 14:53
  • 1
    IMO it would be better to add a class method to `ClientC` that accepts the argument(s) needed and returns an instance of it. This is acceptable because users of the class already know they they want that subclass when creating instances (so presumably also know about the class method only it has). – martineau Aug 05 '17 at 15:00
  • Without any other context, I would claim `BaseClient` should have two methods: `create_with_entity`, which has the same signature as your current `create`, and a new `create` which doesn't take an `entity_id` and is possibly just a wrapper around `create_with_entity`. – chepner Aug 05 '17 at 15:09

3 Answers3

2

I'd say, just add the parameter back as keyword with default value None. Then raise an error that explains that some of the input data is lost.

 class ClientC(BaseClient):
 ....
     def create(self,entity_id=None, data):
         if entity_id:
             raise RedudantInformationError("Value for entity_id does nothing")
         pass

This way whenever a programmer tries to handle child C like the other childs, he'll get a warning reminding him, which however he can easily by-step by using the try-Syntax.

Sanitiy
  • 308
  • 3
  • 8
1

The answer to "can I change signature of child methods?" is yes, nonetheless it is very bad practice.

The children function overriding the parents class must have the same signature, if you want to be SOLID and not violating the LSP.

The example above:

class BaseClient:
    def create(self, entity_id, data=None):
        pass


class EntityBasedClient(BaseClient):
    def create(self, entity_id, data=None):
        pass


class DataBasedClient(BaseClient):
    def create(self, data):
        pass

Is violating the principle and would also raise a linter warning ("Parameters differ from overridden 'create' method")

Also raising a RedudantInformationError, as proposed by @Sanitiy to keep the consistency of the signature, is still violating the principle, as the parent-method would have a different behaviour if used in place of the child-method.

Take a look also at: Python Method overriding, does signature matter?

SeF
  • 3,864
  • 2
  • 28
  • 41
0

I am not sure there is a Pythonic way of doing this, as you can just do as you did in the question. Rather, I would say that this is more about OOP than being Pythonic matter.

So I assume that there are other methods implemented in BaseClient other than create that other children share (otherwise, no point is making ClientC a child of BaseClient). In your case, looks like ClientC is diverging from the rest by requiring a different signature of create method. Then maybe it is the case to consider splitting them?

For example you could have the root BaseClient implement all shared methods except create, and then have two more "base" children, like this:

class EntityBasedClient(BaseClient):
    def create(self, entity_id, data=None):
        pass

class DataBasedClient(BaseClient):
    def create(self, data):
        pass

So now you can inherit without violating any rule:

class ClientA(EntityBasedClient):
     pass

class ClientB(EntityBasedClient):
     pass

class ClientC(DataBasedClient):
    pass

Also, if the create implementation of those two version are pretty similar, you could avoid the code duplication by having a more generic private method implemented in BaseClient with signature _create(self, entity_id=None, data=None), and then call it with appropriate arguments from inside the EntityBasedClient and DataBasedClient.

bagrat
  • 7,158
  • 6
  • 29
  • 47
  • I think this is confusing because previously written functions that expected a BaseClient will now only work with an EntityBasedClient. This makes code difficult to understand. A `create_from_data()` method in ClientC seems more robust. As Martineau said, code that needs to invoke create_from_data() knows it handles a ClientC instance. – Gribouillis Aug 05 '17 at 15:04
  • "...previously written functions that expected a BaseClient will now only work with an EntityBasedClient". Why would that be the case? As I have mentioned, all the shared methods (that also share signature), should be implemented in `BaseClient`, thus will be accessible from any child. Or am I missing something? – bagrat Aug 05 '17 at 15:07
  • You mean that they'll still work as long as they don't call create(). Every function must be reviewed carefully. – Gribouillis Aug 05 '17 at 15:10
  • From OOP perspective, in this case, if a function is expecting a `BaseClient` object, then it should not be supposed to call `create`, as it is ambiguous which implementation would have been called. The same way, if you want to use the `create_from_data` method, than you need to be sure that your object is an instance of `ClientC`. That's the same as making sure that the object is an instance of `DataBasedClient` child. – bagrat Aug 05 '17 at 15:18
  • I suppose your design could be the correct one in certain cases, but we know too little about these Client classes.For example, a function named `create()` could be some sort of constructor and this may change the way it should be handled. That's why I asked the OP to tell us why ClientC needs a different signature. – Gribouillis Aug 05 '17 at 15:33
  • Exactly. I suppose that the OP would not make `ClientC` a child of `BaseClient` just because it is a client. So I assumed that there are other methods in `BaseClient` that all the other client children share. But no matter what's the reason of `ClientC`'s unique requirement, I think in case if its valid, it should be separated as a separate subclass of the base one, as if the creating process and requirements are different, then it makes `ClientC` an actually different conceptual class. – bagrat Aug 05 '17 at 15:37
  • @Gribouillis and @bagrat, of course `BaseClient` contains common methods and abstract property. And in my case all previous clients work good with `create` method before. I also think about @bagrat's approach, but I think about creating some kind of _mixin_ class with implemented `create` mthod – machin Aug 05 '17 at 19:13