Add optional side_effect_meta parameter #31
Conversation
Allow side effects to receive an additional `side_effect_meta` param
which contains the side effect label and the original function's return
value.
This extra param provides a canonical place to store all current and
future side effect metadata, and has a more restricted API than
`return_value`, making it easier to reason about.
1. The `side_effect_meta` param _must_ be an explicit param (i.e. it
cannot be accessed via `**kwargs` like `return_value`).
2. The parameter _must_ be a keyword-only parameter; this avoids issues
with parameter ordering, as well as clashes with `return_value`.
3. The parameter is designed to co-exist with `return_value`, giving
consumers a window to adopt the new API.
| if not ( | ||
| try_bind(func, *args, return_value=return_value, **kwargs) | ||
| try_bind(func, *args, side_effect_meta=meta, **kwargs) | ||
| or try_bind(func, *args, return_value=meta.return_value, **kwargs) |
There was a problem hiding this comment.
We're temporarily adding complexity here, but if the return_value were to be removed in favour of side_effect_meta, we could simplify this signature checking logic considerably.
The return_value bind check could be removed immediately, but also the side_effect_meta check is simpler. Crucially, return_value requires us to check whether the signature can be bound, because it is injected into **kwargs when not explicitly present as a parameter, whereas the need for side_effect_meta can be reduced to checking for a particular parameter name & type in the signature.
This potentially means that signatures could be checked inside the decorators_ has_side_effectsandis_side_effect_of(oncerun_side_effects` was removed) - i.e. on initialisation, rather than at runtime, every time a side effect is triggered.
| @@ -223,27 +232,63 @@ def test_func(): | |||
| self.assertEqual(r.by_label_contains("foo"), {"foo": [test_func]}) | |||
| self.assertEqual(r.by_label_contains("food"), {}) | |||
|
|
|||
| @mock.patch("side_effects.registry._run_func") | |||
There was a problem hiding this comment.
This test has been removed (not rewritten). It had hidden failures, and was incorrect: no_return_value does actually receive a return_value in kwargz.
(The test__run_side_effects__no_return_value test failed silently because _run_func was mocked, but even after removing that, the assertion assert "return_value" not in kwargz actually failed. This failure would not be detected, though, because settings.ABORT_ON_FAILURE is off by default.)
| r = registry.Registry() | ||
|
|
||
| def has_return_value(*args, **kwargs): | ||
| assert "return_value" in kwargs | ||
| actual_call(*args, **kwargs) |
There was a problem hiding this comment.
Using a mock here to monitor the parameters means that the assertion can't be accidentally swallowed by the try/except in _run_func.
Reviewers: this is a draft proposal that would have some utility elsewhere. Any thoughts are welcome.
Add optional side_effect_meta parameter
In some situations it is useful to know why a side effect handler is
called. We could solve this by also passing in the
labelwhen a handleris called, dependent on the signature. Implementation-wise this could be
handled analogously to how the
return_valueis provided, but thisrisks ballooning complexity (e.g. in
try_bind_alland_run_func). Italso fails to provide a simple means of adding extra metadata in future.
Instead, this PR adds a single
side_effect_metaargument, which:currently gives handlers access to the return value and
label;return_value, giving consumers time to switch;**kwargs--this restriction is necessary to avoidcomplexity while
return_valuestill exists, but also results inmore predictable behaviour, and keeps
**kwargsexactly as passedto the original function;
bugs due to parameter ordering (and clashes with
return_value);SideEffectMeta[ReturnValue],and the behaviour is easier to predict from a function signature.