2
\$\begingroup\$

The Problem

I have a module with several sibling classes that share a method with a similar name. These methods receive the same basic arguments, however, for a number of the sibling classes the method in question can take different arguments that are irrelevant to the methods defined in its siblings. For example:

class ElectricKeyboard(Instrument):
    def __init__ (self):
        Instrument.__init__(self)
    def play (sheet_music: list, foot_pedal: bool = True):
        # play some music

class ElectricGuitar(Instrument):
    def __init__ (self):
        Instrument.__init__(self)
    def play (sheet_music: list, foot_pedal: bool = True, guitar_pick: bool = True):
        # play some music

class Kazoo(Instrument):
    def __init__ (self):
        Instrument.__init__(self)
    def play (sheet_music: list):
        # play some music

Above, we have three children of the Instrument class that are siblings. They all have a method, play, that takes a similar argument, however, ElectricKeyboard.play and ElectricGuitar.play take different additional keyword arguments relative to Kazoo.play, which takes none.

Now imagine there exists a separate module where we have some calling context where the guitar_pick keyword argument happens to be defined. Something like this:

# Import the Instrument parent class
from instruments import ElectricGuitar, ElectricKeyboard, Kazoo

# We have some external config file that encapsulates a concert object
import concert_config

# We have some local variables that could be useful
guitar_pick, foot_pedal = True, False

# We initialize all the instruments for the concert and store them in a list
instruments = [Instrument.get_child(instrument)() for instrument in concert_config["instruments"]]

# We then play the music (ignoring that this would play the same bar for each instrument sequentially rather than all at once)
for bar in concert_config["sheet_music"]:
    for instrument in instruments:
        # Either of these three scenarios could occur in the body of the for loop
        instrument.play(bar) # We'd like to do this for the Kazoo
        instrument.play(bar, foot_pedal = foot_pedal) # Or this for the ElectricKeyboard
        instrument.play(bar, foot_pedal = foot_pedal, guitar_pick = guitar_pick) # Or this for the ElectricGuitar

The key here is that in the calling context, the instrument is considered generic; it could be an electric guitar, a keyboard, or a kazoo. We don't care what instrument we're playing, we just want to play it. However, to play it correctly, we want to give as much detail as possible – we would like to provide the values for foot_pedal and guitar_pick, when appropriate. In contrast, if we were to instead have the Kazoo class as our instrument we would not want to pass any additional arguments because they would not be useful or make sense for that instrument.

A Possible Solution

In thinking about how to manage this and preserve the existing architecture (this example is highly contrived relative to the actual application) I thought it might be useful to create a decorator that 'absorbs' the variable scope of the method's calling context (this could be the scope just outside of the method or the global scope).

This is how it would work: Prior to executing the decorated function (i.e. the play method for the instrument), the decorator would 1. retrieve a specified context (e.g. locals()), 2. inspect the function signature to identify its parameters and 3. search the specified context for variables that have the same name as the function parameters and, if found, pass them to the decorated function if they exist. Here's a decorator that does this:

import inspect
from typing import Callable, Any

class AbsorbContext ():
    """
    A decorator that searches a specified context (e.g. locals() or globals())
    for undefined arguments and passes them to the decorated function from the
    local contest if they are defined there (i.e. 'absorbs' them).
    """
    def __init__ (self, 
        context: dict = globals(), 
        positional_only: bool = True, 
        positional_or_keyword: bool = True, 
        keyword_only: bool = True
    ):
        self.positional_only = positional_only
        self.positional_or_keyword = positional_or_keyword
        self.keyword_only = keyword_only
        self.context = context
    def __call__ (self, func: Callable[..., Any]):
        def absorb (*args, **kwargs):
            params = inspect.signature(func).parameters.values()
            if self.positional_only:
                absorbed_pos_args = ()
                pos_only = [param.name for param in params if param.kind is inspect.Parameter.POSITIONAL_ONLY]
                args = tuple(self.context[arg] for arg in pos_only if arg in self.context)
            if self.positional_or_keyword:
                absorbed_pos_or_kwd_args = {}
                pos_or_kwd = [param.name for param in params if param.kind is inspect.Parameter.POSITIONAL_OR_KEYWORD]
                kwargs = dict(kwargs, **{arg: self.context[arg] for arg in pos_or_kwd if arg in self.context})
            if self.keyword_only:
                absorbed_kwd_args = {}
                kwd_only = [param.name for param in params if param.kind is inspect.Parameter.KEYWORD_ONLY]
                kwargs = dict(kwargs, **{arg: self.context[arg] for arg in kwd_only if arg in self.context})
            return func(*args, **kwargs)
        return absorb

This works and seems to achieved the desired behavior. Below is an example usage (if you're unfamiliar with the / and * syntax seen in function signature below, see this answer).

Example Usage

# Some Arguments
a = 2
b = 3
c = 4
d = 5

@AbsorbContext(context = locals())
def func (a: int, b: int, /, c: int = 0, *, d: int = 1):
    return (a * b + c) * d

func(a) # Returns 50

func(a, b) # Returns 50

func() # We can pass nothing and it will still evaluate correctly; returns 50

# ...

As long as we maintain the correct ordering for args a and b (the positional only arguments), the method will always return the correct value, 50, given the parameters that are available for input in the calling context. This can also be thought of as defining the default argument values for a function in the calling context rather than in the function signature.

func(1, 2, 3, 4) # All new arguments, returns 20

func(1) # Modify a POSITION_ONLY argument, returns 35

func(d = 10) # Modify a KEYWORD_ONLY argument, returns 100

func(2, 3, c = 14) # Modify a POSITION_OR_KEYWORD argument, returns 100

func(2, 3, 14) # Modify a POSITION_OR_KEYWORD argument, returns 100

Note that unlike in the example shown here, in general the definition of the function (e.g. func) will exist in a separate module from the one where the arguments are defined and the function is called.

The Question

This feels like a hack. While this seems to work I have some questions:

  1. Is there a way to get a similar behavior that does not require a decorator of this sort? It seems like this could be a weakness with the architecture I have chosen – are there any architectures that have been designed to solve this problem?

  2. Is there any obvious way this behavior could be exploited by a bad actor? Note that this software is not designed to run or be called over a network; it is reasonable to assume that all arguments will be defined by the user at runtime.

  3. In its current form, the context should be called in the same module that the decorated function is defined. Is there a way to make this more flexible? I think we might be able to solve this problem by calling globals(), but that seems inelegant.

\$\endgroup\$
  • \$\begingroup\$ Please see What to do when someone answers. I have rolled back Rev 4 → 3 \$\endgroup\$ – Sᴀᴍ Onᴇᴌᴀ May 11 at 22:23
  • \$\begingroup\$ @SᴀᴍOnᴇᴌᴀ Gotcha, thanks! \$\endgroup\$ – Greenstick May 11 at 22:24
  • \$\begingroup\$ feel free to re-add the words added to question two (i.e. it is reasonable to assume that) if you so desire \$\endgroup\$ – Sᴀᴍ Onᴇᴌᴀ May 11 at 22:26
3
\$\begingroup\$

It definitely seems like a hack.

  • A first smell is that changing the function signature by just renaming variables will break it. If a is available in locals(), it has to match a in the function signature. Even just a capital A in either spot will break the behaviour.

    This requires you to change names in multiple locations if you only wanted to change it in one.

  • Next, it is hard to follow and debug. It is certainly very surprising behaviour to anyone just getting to know your code.

    Your functions return surprising results that no longer correspond to the very arguments the caller provided. Instead, they are influenced by global state and cannot be overridden:

      # Some Arguments
      a = 2
      b = 3
      c = 4
      d = 5
    
      @AbsorbContext(context = locals())
      def func (a: int, b: int, /, c: int = 0, *, d: int = 1):
          return (a * b + c) * d
    
      print(func(1, 2, 1, 1))  # Expected to return 3, returns 50
    
  • Variable names like a, b, c, ... make sense now, but in a larger context, more elaborate variable names are needed. These are then blocked for usage, and a person declaring new variables has to check every decorated function for collisions.

    Not only that, also def and class definitions have to be regarded, since these also bind to names. Built-ins can also collide, though function parameters shadowing built-ins is a terrible idea and rare. Same is true for import *: terrible idea in the first place, but @AbsorbContext turns it into proper chaos.

    Situations like this can arise (fails because check_array is a function):

      def check_array():
        pass
    
      # check_array = True  # uncommenting works
    
      @AbsorbContext(context = locals())
      def func (a: int, check_array: bool, /, c: int = 0, *, d: int = 1):
          return (a * int(check_array) + c) * d
    

    check_array is a generic name which is easily imagined to be that of a function, or a function parameter.

  • Providing context=globals() as a default argument to __init__ will use globals() of the module where AbsorbContext is defined. This will break behaviour if that class is imported, which you will probably be doing. context should not have a default argument.
  • AbsorbContent could have been a function and therefore shorter. Did you leverage self to access state? Decorator functions can do this through closure.
  • Situations in which run-time should definitely error out are silently overridden:

      # Some Arguments
      a = 2
      b = 3
      c = 4
      d = 5
    
      @AbsorbContext(context = locals())
      def func (a: int, b: int, /, c: int = 0, *, d: int = 1):
          return (a * b + c) * d
    
      print(func(1, 2, 1, 1, 1, 1, 1, 1, 1, 1))  # Expected to error out
    

    Here, a TypeError for mismatched function arguments and parameters is expected, but it works and returns 50. This bug might not be found immediately, even though it should definitely fail-fast.


In the spirit of your "absorption" approach, you could use **kwargs in play to collect (absorb) all unused keyword arguments which the function has no use for. sheet_music then remains as a mandatory positional argument in all cases:

class Instrument:
    pass


class ElectricKeyboard(Instrument):
    def __init__ (self):
        Instrument.__init__(self)
    def play (self, sheet_music: list, foot_pedal: bool = True, **kwargs):
        print("Playing Keyboard")
        # play some music


class ElectricGuitar(Instrument):
    def __init__ (self):
        Instrument.__init__(self)
    def play (self, sheet_music: list, foot_pedal: bool = True, guitar_pick: bool = True, **kwargs):
        print("Playing Guitar")
        # play some music


class Kazoo(Instrument):
    def __init__ (self):
        Instrument.__init__(self)
    def play (self, sheet_music: list, **kwargs):
        print("Playing Kazoo")
        # play some music


instruments = [ElectricGuitar(), ElectricKeyboard(), Kazoo()]

for instrument in instruments:
    instrument.play("sheet_music", foot_pedal=True)
    instrument.play("sheet_music")
    instrument.play("sheet_music", guitar_pick=True)

Now, all those ducks quack properly.


Collecting instruments and then iterating over them calls for identical interfaces. This is because lists are homogeneous. They should contain items of identical type (think of a list of text files; you can safely call .read() on all of these). This is another hint that the sibling approach might be off.

Instead, you could look into composition and implement a MusicGroup class with has-a relations towards the instruments played by the musical group. MusicGroup then has methods like play_guitars to play all available guitars. play_guitars can have a specialized signature, which only makes sense for guitars. Then, you can give MusicGroup a play or maybe play_all method to call all play_<instrument> methods. The play_all method would forward all required **kwargs, or better still, manually forward them to each specific function.

Your inheritances and attempt to treat all siblings equally might be a case of the Circle-Ellipse problem. Your instruments certainly all fulfill the is-a relationship towards Instrument, but that does not necessarily warrant inheritance if their behaviours differ too much.

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ Thanks this is a very nice overview of some problems with the concept. I'll continue to think on the structure of the sibling classes – this was a fun experiment but it definitely won't make it into the library! \$\endgroup\$ – Greenstick May 11 at 22:11

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.