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:
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?
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.
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.
-
\$\begingroup\$ Please see What to do when someone answers. I have rolled back Rev 4 → 3 \$\endgroup\$Sᴀᴍ Onᴇᴌᴀ– Sᴀᴍ Onᴇᴌᴀ ♦2020年05月11日 22:23:32 +00:00Commented May 11, 2020 at 22:23
-
\$\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ᴇᴌᴀ– Sᴀᴍ Onᴇᴌᴀ ♦2020年05月11日 22:26:12 +00:00Commented May 11, 2020 at 22:26
1 Answer 1
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 inlocals()
, it has to matcha
in the function signature. Even just a capitalA
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
andclass
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 forimport *
: 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 useglobals()
of the module whereAbsorbContext
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 leverageself
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 returns50
. 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.
-
\$\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– Greenstick2020年05月11日 22:11:19 +00:00Commented May 11, 2020 at 22:11