ashleysommer / sanic-plugin-toolkit

Easily create Plugins for Sanic!
MIT License
51 stars 9 forks source link

handling of arguments in SanicPlugin prevents multiple inheritance #13

Closed fmux closed 5 years ago

fmux commented 5 years ago

The __init__() method of the SanicPlugin class checks for the presence of any arguments using the following lines (lines 429-437 in plugin.py at tag 0.8.2:

def __init__(self, *args, **kwargs):
    # Sometimes __init__ can be called twice.
    # Ignore it on subsequent times
    if self._initialized:
        return
    assert len(args) < 1,\
        "Unexpected arguments passed to this Sanic Plugins."
    assert len(kwargs) < 1,\
        "Unexpected keyword arguments passed to this Sanic Plugins."

However, in case of multiple inheritance, it may actually be necessary to pass on arguments to the next class in the method resolution order, as in the following proof-of-concept example:

from spf import SanicPlugin

class SecretKeyStore:
    def __init__(self, secret_key, *args, **kwargs):
        super().__init__(*args, **kwargs)
        self.secret_key = secret_key

class MyPlugin(SanicPlugin, SecretKeyStore):
    def __init__(self, *args, **kwargs):
        super().__init__(*args, **kwargs)

# secret key might be read from a plugin-specific config file
my_plugin = MyPlugin(secret_key="abcd1234")

Note that in this case it would still be possible to get the code to run by swapping the order of the base classes, however as soon as one has more than one base class that handles arguments the way SanicPlugin does, one is out of luck.

Is there any reason why the two assertions are necessary? If not, I believe they can be simply deleted. If the only remaining class in the method resolution order is object, it will complain anyway if there are any arguments that were not accounted for.

ashleysommer commented 5 years ago

Hi @fmux Thanks for bringing this up.

I believe I had those extra assertions in there to help users find a problem, in the case when they are accidentally passing parameters to the constructor of the plugin definition, rather than the initializer of a plugin instance. I can see that would be a common mistake for new users to make, who have never developed a plugin with SPF before.

I did cross my mind that this might cause problems for multiple inheritance, but in the past I've always solved in the way you pointed out, by swapping them around. But you're right, this will definitely cause a problem in the case that more than one class restricts the args passed in.

I believe this can simply be removed. Please feel free to submit a PR with the changes, and I will merge it.