Closed jsenecal closed 4 years ago
Hello @jsenecal,
thank you for that pull request. I really appreciate that you added tests and already documented your feature. Great contribution! I think that's a feature worth supporting.
However, I strive to keep the impact of new features (and often not so common use cases) rather low for all existing users. Having to ALWAYS check for a possible property in every callback resolution in addition to the increased complexity of the resolve_callback
function itself feels a bit heavy considering that most user do not make use of this.
So I came up with two alternatives:
"If its not callable, wrap it into a function"
def duck_resolve_callable(func, model):
if isinstance(func, string_types):
try:
func = getattr(model, func)
if not callable(func):
def func_wrapper(*args, **kwargs):
return func
return func_wrapper
except AttributeError:
func = None
return func
"Assume a callable return value and return the returned value when this assumption was wrong"
# class Condition
@staticmethod
def check(func, event_data):
# [....]
predicate= event_data.machine.resolve_callable(self.func, event_data)
if self.send_event:
try:
return predicate(event_data) == self.target
except TypeError:
return func == target
I'd expect EAFP to have the lowest impact on existing users (no property callbacks) with a notable increase in code complexity though and some responsibility leaks (callers have to deal with not callable properties/attributes). Duck version looks a bit nicer imho but resolves/calls the property in resolve_callback
already. Since resolve_callback
is always executed right before the actual callback, this seems to be an acceptable trade-off (properties/attributes cannot handle parameters anyway). I also did a performance benchmark [1] (n=1,000,000) for all three candidates:
The lower bar only considers callback resolution while the whole bar is the time including callback execution (a function/property just returning true). We can see that for currently supported cases, EAFP performs best. If about a third of the callback calls are actually properties, Duck version takes the first place. However, I am not a big fan of try/except-fencing every execution of a return value of resolve_callback
though. Based on this I pulled your PR into dev-0.8
and implemented the "duck version" for callback/property/attribute resolution.
But since you suggested that feature and probably have certain use cases in mind, I wanted to ask you for feedback before I merge the dev branch into master
.
Includes tests for new functionality