From 7ab2d07f8f1fff4ca411d4ead13f9d94cbda8648 Mon Sep 17 00:00:00 2001 From: Hugo Rodger-Brown Date: Sun, 6 Nov 2022 13:22:04 +0000 Subject: [PATCH 1/2] Simplify binding --- .gitignore | 1 + CHANGELOG | 9 ++ side_effects/decorators.py | 6 +- .../commands/display_side_effects.py | 2 +- side_effects/registry.py | 98 +++++----------- tests/__init__.py | 8 +- tests/test_decorators.py | 2 +- tests/test_registry.py | 105 +++--------------- 8 files changed, 64 insertions(+), 167 deletions(-) create mode 100644 CHANGELOG diff --git a/.gitignore b/.gitignore index e118187..7ff0fc3 100644 --- a/.gitignore +++ b/.gitignore @@ -4,3 +4,4 @@ *.egg-info *.lock *.pyc +dist/ diff --git a/CHANGELOG b/CHANGELOG new file mode 100644 index 0000000..015e9f7 --- /dev/null +++ b/CHANGELOG @@ -0,0 +1,9 @@ +# Changelog + +All notable changes to this project will be documented in this file. + +## v2.0 (pre-release) + +* Add support for Python 3.11 +* Drop support for Django 3.7 +* Remove `run_on_exit` param from `has_side_effects` diff --git a/side_effects/decorators.py b/side_effects/decorators.py index 62a4da7..3c87628 100644 --- a/side_effects/decorators.py +++ b/side_effects/decorators.py @@ -56,7 +56,7 @@ def has_side_effects( def decorator(func: Callable) -> Callable: @wraps(func) - def inner_func(*args: Any, **kwargs: Any) -> Any: + def inner_func(*args: object, **kwargs: object) -> Any: """Run the original function and send the signal if successful.""" return_value = func(*args, **kwargs) if run_on_exit(return_value): @@ -77,7 +77,7 @@ def decorator(func: Callable) -> Callable: registry.register_side_effect(label, func) @wraps(func) - def inner_func(*args: Any, **kwargs: Any) -> Any: + def inner_func(*args: object, **kwargs: object) -> Any: return func(*args, **kwargs) return inner_func @@ -90,7 +90,7 @@ def disable_side_effects() -> Callable: def decorator(func: Callable) -> Callable: @wraps(func) - def inner_func(*args: Any, **kwargs: Any) -> Any: + def inner_func(*args: object, **kwargs: object) -> Any: with registry.disable_side_effects() as events: return func(*args, events, **kwargs) diff --git a/side_effects/management/commands/display_side_effects.py b/side_effects/management/commands/display_side_effects.py index 72b5571..a0c5f7b 100644 --- a/side_effects/management/commands/display_side_effects.py +++ b/side_effects/management/commands/display_side_effects.py @@ -14,7 +14,7 @@ class Command(BaseCommand): help = "Displays project side_effects." - def __init__(self, *args: Any, **kwargs: Any) -> None: + def __init__(self, *args: object, **kwargs: object) -> None: self.missing_docstrings = [] # type: List[str] super().__init__(*args, **kwargs) diff --git a/side_effects/registry.py b/side_effects/registry.py index da427f1..72c1f1f 100644 --- a/side_effects/registry.py +++ b/side_effects/registry.py @@ -105,17 +105,7 @@ def add(self, label: str, func: Callable) -> None: with self._lock: self[label].append(func) - def _run_side_effects( - self, label: str, *args: Any, return_value: Any | None = None, **kwargs: Any - ) -> None: - if settings.TEST_MODE_FAIL: - raise SideEffectsTestFailure(label) - for func in self[label]: - _run_func(func, *args, return_value=return_value, **kwargs) - - def run_side_effects( - self, label: str, *args: Any, return_value: Any | None = None, **kwargs: Any - ) -> None: + def run_side_effects(self, label: str, *args: object, **kwargs: object) -> None: """ Run registered side-effects functions, or suppress as appropriate. @@ -124,33 +114,36 @@ def run_side_effects( primarily used by the disable_side_effects context manager to register which side-effects events were suppressed (for testing purposes). - NB even if the side-effects themselves are not run, this method will try - to bind all of the receiver functions - this is to ensure that incompatible - functions fail hard and early. - """ - # TODO: this is all becoming over-complex - need to simplify this - self.try_bind_all(label, *args, return_value=return_value, **kwargs) if self._suppress or settings.TEST_MODE: self.suppressed_side_effect.send(Registry, label=label) - else: - self._run_side_effects(label, *args, return_value=return_value, **kwargs) - - def try_bind_all( - self, label: str, *args: Any, return_value: Any | None = None, **kwargs: Any - ) -> None: - """ - Test all receivers for signature compatibility. - - Raise SignatureMismatch if any function does not match. - - """ + return + if settings.TEST_MODE_FAIL: + raise SideEffectsTestFailure(label) for func in self[label]: - if not ( - try_bind(func, *args, return_value=return_value, **kwargs) - or try_bind(func, *args, **kwargs) - ): - raise SignatureMismatch(func) + self.try_bind(func, *args, **kwargs) + self.run_func(func, *args, **kwargs) + + def run_func(self, func: Callable, *args: object, **kwargs: object) -> None: + try: + func(*args, **kwargs) + except Exception: # noqa: B902 + logger.exception("Error running side_effect function '%s'", fname(func)) + if settings.ABORT_ON_ERROR or settings.TEST_MODE_FAIL: + raise + + def try_bind(self, func: Callable, *args: object, **kwargs: object) -> None: + """Try binding args & kwargs to a given func.""" + try: + inspect.signature(func).bind(*args, **kwargs) + except TypeError: + # if the func isn't expecting return_value (which we have added + # to the kwargs in the has_side_effects decorator), then try + # again without it. + if "return_value" in kwargs: + kwargs.pop("return_value", None) + return self.try_bind(func, *args, **kwargs) + raise SignatureMismatch(func) class disable_side_effects: @@ -189,9 +182,7 @@ def register_side_effect(label: str, func: Callable) -> None: _registry.add(label, func) -def run_side_effects( - label: str, *args: Any, return_value: Any | None = None, **kwargs: Any -) -> None: +def run_side_effects(label: str, *args: object, **kwargs: object) -> None: """Run all of the side-effect functions registered for a label.""" if not transaction.get_autocommit(): getattr(logger, settings.ATOMIC_TX_LOG_LEVEL)( @@ -199,38 +190,7 @@ def run_side_effects( "transaction. This may have unintended consequences.", label, ) - _registry.run_side_effects(label, *args, return_value=return_value, **kwargs) - - -def _run_func( - func: Callable, *args: Any, return_value: Any | None = None, **kwargs: Any -) -> None: - """Run a single side-effect function and handle errors.""" - try: - if try_bind(func, *args, return_value=return_value, **kwargs): - func(*args, return_value=return_value, **kwargs) - elif try_bind(func, *args, **kwargs): - func(*args, **kwargs) - else: - raise SignatureMismatch(func) - except SignatureMismatch: - # always re-raise SignatureMismatch as this means we have been unable - # to run the side-effect function at all. - raise - except Exception: # noqa: B902 - logger.exception("Error running side_effect function '%s'", fname(func)) - if settings.ABORT_ON_ERROR or settings.TEST_MODE_FAIL: - raise - - -def try_bind(func: Callable, *args: Any, **kwargs: Any) -> bool: - """Try binding args & kwargs to a given func.""" - try: - inspect.signature(func).bind(*args, **kwargs) - except TypeError: - return False - else: - return True + _registry.run_side_effects(label, *args, **kwargs) # global registry diff --git a/tests/__init__.py b/tests/__init__.py index 6ee8be2..6447de8 100644 --- a/tests/__init__.py +++ b/tests/__init__.py @@ -4,19 +4,19 @@ @has_side_effects("foo") def origin(message: str) -> str: - print(f"origin: {message}") # noqa: T001 + print(f"origin: {message}") # noqa: T201 return f"Message received: {message}" @is_side_effect_of("foo") def no_docstring(message: str) -> None: - print(f"side-effect.1: message={message}") # noqa: T001 + print(f"side-effect.1: message={message}") # noqa: T201 @is_side_effect_of("foo") def one_line_docstring(message: str) -> None: """This is a one-line docstring.""" - print(f"side-effect.2: message={message}") # noqa: T001 + print(f"side-effect.2: message={message}") # noqa: T201 @is_side_effect_of("foo") @@ -27,6 +27,6 @@ def multi_line_docstring(message: str, return_value: str) -> None: It has more information here. """ - print( # noqa: T001 + print( # noqa: T201 f"Side-effect.3: message={message}, return_value={return_value}" ) diff --git a/tests/test_decorators.py b/tests/test_decorators.py index 0adcf4c..6899a05 100644 --- a/tests/test_decorators.py +++ b/tests/test_decorators.py @@ -94,7 +94,7 @@ def foo(): class ContextManagerTests(TestCase): - @mock.patch("side_effects.registry._run_func") + @mock.patch.object(registry.Registry, "run_func") def test_disable_side_effects(self, mock_func): """Side-effects can be temporarily disabled.""" diff --git a/tests/test_registry.py b/tests/test_registry.py index 8c400d7..07a11b9 100644 --- a/tests/test_registry.py +++ b/tests/test_registry.py @@ -68,11 +68,13 @@ def foo4(return_value, **kwargs): def foo5(arg1, **kwargs): pass - self.assertTrue(registry.try_bind(foo1, return_value=1)) - self.assertTrue(registry.try_bind(foo2, 1, return_value=1)) - self.assertTrue(registry.try_bind(foo3, 1, 2, 3, return_value=1)) - self.assertTrue(registry.try_bind(foo4, bar="baz", return_value=1)) - self.assertTrue(registry.try_bind(foo5, 1, return_value=1)) + r = registry.Registry() + r.try_bind(foo1, return_value=1) + r.try_bind(foo2, 1, return_value=1) + r.try_bind(foo3, 1, 2, 3, return_value=1) + r.try_bind(foo4, bar="baz", return_value=1) + r.try_bind(foo5, 1, return_value=1) + self.assertRaises(registry.SignatureMismatch, r.try_bind, foo1, 1, 2) def test_try_bind__without_return_value(self): def foo1(): @@ -84,9 +86,11 @@ def foo2(arg1): def foo3(*args): pass - self.assertFalse(registry.try_bind(foo1, return_value=1)) - self.assertFalse(registry.try_bind(foo2, 1, return_value=1)) - self.assertFalse(registry.try_bind(foo3, 1, 2, 3, return_value=1)) + r = registry.Registry() + r.try_bind(foo1, return_value=1) + r.try_bind(foo2, 1, return_value=1) + r.try_bind(foo3, 1, 2, 3, return_value=1) + self.assertRaises(registry.SignatureMismatch, r.try_bind, foo1, 1) def test_register_side_effect(self): def test_func1(): @@ -102,7 +106,7 @@ def test_func2(): registry.register_side_effect("foo", test_func1) self.assertTrue(registry._registry.contains("foo", test_func1)) - @mock.patch("side_effects.registry._run_func") + @mock.patch.object(registry.Registry, "run_func") def test_run_side_effects(self, mock_func): def test_func1(): pass @@ -160,26 +164,7 @@ def test_func(): "foo", ) - def test__run_func__no_return_value(self): - """Test the _run_func function does not pass return_value if not required.""" - - def test_func(): - pass - - registry._run_func(test_func, return_value=None) - - def test__run_func__with_return_value(self): - """Test the _run_func function passes through the return_value if required.""" - - def test_func(**kwargs): - assert "return_value" in kwargs - - # return_value not passed through, so fails - registry._run_func(test_func) - # self.assertRaises(KeyError, registry._run_func, test_func) - registry._run_func(test_func, return_value=None) - - def test__run_func__aborts_on_error(self): + def test_run_func__aborts_on_error(self): """Test the _run_func function handles ABORT_ON_ERROR correctly.""" def test_func(): @@ -188,23 +173,12 @@ def test_func(): # error is logged, but not raised with mock.patch.object(settings, "ABORT_ON_ERROR", False): self.assertFalse(settings.ABORT_ON_ERROR) - registry._run_func(test_func, return_value=None) + registry._registry.run_func(test_func, return_value=None) # error is raised with mock.patch.object(settings, "ABORT_ON_ERROR", True): self.assertTrue(settings.ABORT_ON_ERROR) - self.assertRaises(Exception, registry._run_func, test_func) - - def test__run_func__signature_mismatch(self): - """Test the _run_func function always raises SignatureMismatch.""" - - def test_func(): - raise Exception("Pah") - - with mock.patch.object(settings, "ABORT_ON_ERROR", False): - self.assertRaises( - registry.SignatureMismatch, registry._run_func, test_func, 1 - ) + self.assertRaises(Exception, registry._registry.run_func, test_func) class RegistryTests(TestCase): @@ -242,50 +216,3 @@ def test_func(): self.assertEqual(r.by_label_contains("fo"), {"foo": [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") - def test__run_side_effects__no_return_value(self, mock_run): - """Test return_value is not passed""" - - def no_return_value(*args, **kwargz): - assert "return_value" not in kwargz - - r = registry.Registry() - r.add("foo", no_return_value) - r._run_side_effects("foo") - r._run_side_effects("foo", return_value=None) - - def test__run_side_effects__with_return_value(self): - """Test return_value is passed""" - r = registry.Registry() - - def has_return_value(*args, **kwargs): - assert "return_value" in kwargs - - r.add("foo", has_return_value) - r._run_side_effects("foo", return_value=None) - - def test_try_bind_all(self): - def foo1(return_value): - pass - - def foo2(arg1, return_value): - pass - - def foo3(*args, return_value): - pass - - def foo4(return_value, **kwargs): - pass - - def foo5(arg1, **kwargs): - pass - - r = registry.Registry() - r.add("foo", foo1) - r.add("foo", foo2) - r.add("foo", foo3) - r.add("foo", foo4) - r.add("foo", foo5) - r.try_bind_all("foo", 1) - self.assertRaises(registry.SignatureMismatch, r.try_bind_all, "foo", 1, 2) From 54e48a436f75b7d438495d56a596b61fa1caba45 Mon Sep 17 00:00:00 2001 From: Hugo Rodger-Brown Date: Sun, 6 Nov 2022 13:27:09 +0000 Subject: [PATCH 2/2] Remove run_on_exit param --- README.rst | 19 ------------------- side_effects/decorators.py | 23 +++-------------------- tests/test_decorators.py | 13 ------------- 3 files changed, 3 insertions(+), 52 deletions(-) diff --git a/README.rst b/README.rst index d95a266..d983dc0 100644 --- a/README.rst +++ b/README.rst @@ -97,25 +97,6 @@ that has side effects: def foo(*args, **kwargs): pass -**Decorating view functions** - -By default, the ``has_side_effects`` decorator will run so long as the inner -function does not raise an exception. View functions, however, are a paticular -case where the function may run, and return a perfectly valid ``HttpResponse`` -object, but you do **not** want the side effects to run, as the response object -has a ``status_code`` of 404, 500, etc. In this case, you want to inspect the -inner function return value before deciding whether to fire the side effects -functions. In order to support this, the ``has_side_effects`` decorator has -a kwarg ``run_on_exit`` which takes a function that takes a single parameter, -the return value from the inner function, and must return ``True`` or ``False`` -which determines whether to run the side effects. - -The ``decorators`` module contains the default argument for this kwarg, a -function called ``http_response_check``. This will return ``False`` if the -inner function return value is an ``HttpResponse`` object with a status -code in the 4xx-5xx range. - - The second decorator, ``is_side_effect_of``, is used to bind those functions that implement the side effects to the origin function: diff --git a/side_effects/decorators.py b/side_effects/decorators.py index 3c87628..27cb446 100644 --- a/side_effects/decorators.py +++ b/side_effects/decorators.py @@ -16,9 +16,7 @@ def http_response_check(response: HttpResponse) -> bool: return True -def has_side_effects( - label: str, run_on_exit: Callable = http_response_check -) -> Callable: +def has_side_effects(label: str) -> Callable: """ Run decorated function and raise side_effects signal when complete. @@ -34,24 +32,11 @@ def has_side_effects( module in core. You can use the label to call the appropriate side effect function. - The run_on_exit kwarg can be used for fine-grained control over the exact - behaviour required. The canonical use case for this is when decorating - view functions - as they will typically always return a valid HttpResponse - object, and use the status_code property to indicate whether the view - function ran OK. - Args: label: string, an identifier that is used in the receiver to determine which event has occurred. This is required because the function name won't be sufficient in most cases. - Kwargs: - run_on_exit: function used to determine whether the side effects should - run, based on the return value of the innner function. This can be - used to inspect the result for fine grained control. The default is - `http_response_check`, which will return False for 4xx, 5xx status - codes. - """ def decorator(func: Callable) -> Callable: @@ -59,10 +44,8 @@ def decorator(func: Callable) -> Callable: def inner_func(*args: object, **kwargs: object) -> Any: """Run the original function and send the signal if successful.""" return_value = func(*args, **kwargs) - if run_on_exit(return_value): - registry.run_side_effects( - label, *args, return_value=return_value, **kwargs - ) + kwargs["return_value"] = return_value + registry.run_side_effects(label, *args, **kwargs) return return_value return inner_func diff --git a/tests/test_decorators.py b/tests/test_decorators.py index 6899a05..a62c954 100644 --- a/tests/test_decorators.py +++ b/tests/test_decorators.py @@ -38,19 +38,6 @@ def test_func(arg1: int): func(1) mock_registry.run_side_effects.assert_called_with("foo", 1, return_value=2) - @mock.patch("side_effects.decorators.registry") - def test_has_side_effects__run_on_exit_false(self, mock_registry): - """Decorated functions should call run_side_effects.""" - - def test_func(*args, **kwargs): - pass - - func = decorators.has_side_effects("foo", run_on_exit=lambda r: False)( - test_func - ) - func("bar") - mock_registry.run_side_effects.assert_not_called() - @mock.patch("side_effects.registry.register_side_effect") def test_is_side_effect_of(self, mock_register): """Decorated functions should be added to the registry."""