From 4cf2ded851cd9ad3df98a16149568cf15a8a4a58 Mon Sep 17 00:00:00 2001 From: Mark Dickinson Date: Wed, 15 Sep 2021 14:07:40 +0100 Subject: [PATCH 1/6] Warn on a bad signature for an 'observe'-decorated function --- traits/has_traits.py | 21 +++++++++++++++++++++ traits/tests/test_observe.py | 14 ++++++++++++++ 2 files changed, 35 insertions(+) diff --git a/traits/has_traits.py b/traits/has_traits.py index ece84b1f7..42727a72d 100644 --- a/traits/has_traits.py +++ b/traits/has_traits.py @@ -841,6 +841,27 @@ def observe_decorator(handler): handler : callable Method of a subclass of HasTraits """ + # Warn on a missing 'event' parameter in the handler. The handler + # should accept a call consisting of a single positional argument. + try: + handler_signature = inspect.signature(handler) + except (ValueError, TypeError): + pass + else: + try: + handler_signature.bind("self", "event") + except TypeError: + warnings.warn( + ( + "Suspicious signature for observe-decorated method. " + "The target method should be callable with a single " + "positional parameter. Did you forget to add an " + "'event' parameter?" + ), + UserWarning, + stacklevel=2, + ) + try: observe_inputs = handler._observe_inputs except AttributeError: diff --git a/traits/tests/test_observe.py b/traits/tests/test_observe.py index 22f2a55da..8f05398a3 100644 --- a/traits/tests/test_observe.py +++ b/traits/tests/test_observe.py @@ -37,6 +37,20 @@ ) +class TestObserveDecorator(unittest.TestCase): + """ General tests for the observe decorator. """ + + def test_warning_on_handler_with_bad_signature(self): + message_regex = "should be callable with a single positional parameter" + with self.assertWarnsRegex(UserWarning, message_regex): + class A(HasTraits): + foo = Int() + + @observe("foo") + def _do_something_when_foo_changes(self): + pass + + class Student(HasTraits): """ Model for testing list + post_init (enthought/traits#275) """ From d176c1bc42979a18098c0aaad2e7f75f1154e22e Mon Sep 17 00:00:00 2001 From: Mark Dickinson Date: Wed, 15 Sep 2021 14:37:31 +0100 Subject: [PATCH 2/6] Wording tweaks --- traits/has_traits.py | 16 +++++++++------- traits/tests/test_observe.py | 2 +- 2 files changed, 10 insertions(+), 8 deletions(-) diff --git a/traits/has_traits.py b/traits/has_traits.py index 42727a72d..6c9f5b586 100644 --- a/traits/has_traits.py +++ b/traits/has_traits.py @@ -839,10 +839,12 @@ def observe_decorator(handler): Parameters ---------- handler : callable - Method of a subclass of HasTraits + Method of a subclass of HasTraits, with signature of the form + ``my_method(self, event)``. """ - # Warn on a missing 'event' parameter in the handler. The handler - # should accept a call consisting of a single positional argument. + # Warn on a dubious handler signature. The handler should accept a call + # that passes a single positional argument (conventionally named + # "event") in addition to the usual "self". try: handler_signature = inspect.signature(handler) except (ValueError, TypeError): @@ -853,10 +855,10 @@ def observe_decorator(handler): except TypeError: warnings.warn( ( - "Suspicious signature for observe-decorated method. " - "The target method should be callable with a single " - "positional parameter. Did you forget to add an " - "'event' parameter?" + "Dubious signature for observe-decorated method. " + "The decorated method should be callable with a " + "single positional argument in addition to 'self'. " + "Did you forget to add an 'event' parameter?" ), UserWarning, stacklevel=2, diff --git a/traits/tests/test_observe.py b/traits/tests/test_observe.py index 8f05398a3..3539e27f5 100644 --- a/traits/tests/test_observe.py +++ b/traits/tests/test_observe.py @@ -41,7 +41,7 @@ class TestObserveDecorator(unittest.TestCase): """ General tests for the observe decorator. """ def test_warning_on_handler_with_bad_signature(self): - message_regex = "should be callable with a single positional parameter" + message_regex = "should be callable with a single positional argument" with self.assertWarnsRegex(UserWarning, message_regex): class A(HasTraits): foo = Int() From 9d32b099530e189d3de22756ffce77a844ba9011 Mon Sep 17 00:00:00 2001 From: Mark Dickinson Date: Wed, 15 Sep 2021 14:42:22 +0100 Subject: [PATCH 3/6] Expand tests --- traits/tests/test_observe.py | 35 +++++++++++++++++++++++++++++++++++ 1 file changed, 35 insertions(+) diff --git a/traits/tests/test_observe.py b/traits/tests/test_observe.py index 3539e27f5..f1ef07a89 100644 --- a/traits/tests/test_observe.py +++ b/traits/tests/test_observe.py @@ -42,6 +42,7 @@ class TestObserveDecorator(unittest.TestCase): def test_warning_on_handler_with_bad_signature(self): message_regex = "should be callable with a single positional argument" + with self.assertWarnsRegex(UserWarning, message_regex): class A(HasTraits): foo = Int() @@ -50,6 +51,40 @@ class A(HasTraits): def _do_something_when_foo_changes(self): pass + with self.assertWarnsRegex(UserWarning, message_regex): + class A(HasTraits): + foo = Int() + + @observe("foo") + def _do_something_when_foo_changes(self, **kwargs): + pass + + def test_decorated_method_signatures(self): + # Test different handler signatures for compatibility with + # observe decorator. + + class A(HasTraits): + foo = Int() + + call_count = Int(0) + + @observe("foo") + def _method_with_extra_optional_args(self, event, frombicate=True): + self.call_count += 1 + + @observe("foo") + def _method_with_star_args(self, *args): + self.call_count += 1 + + @observe("foo") + def _method_with_alternative_name(self, foo_change_event): + self.call_count += 1 + + a = A() + self.assertEqual(a.call_count, 0) + a.foo = 23 + self.assertEqual(a.call_count, 3) + class Student(HasTraits): """ Model for testing list + post_init (enthought/traits#275) """ From 3b74aea9a66b28d9c768ac6b2422b467ba1936f9 Mon Sep 17 00:00:00 2001 From: Mark Dickinson Date: Wed, 15 Sep 2021 14:47:17 +0100 Subject: [PATCH 4/6] Fix flake8 complaint --- traits/tests/test_observe.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/traits/tests/test_observe.py b/traits/tests/test_observe.py index f1ef07a89..f0883e274 100644 --- a/traits/tests/test_observe.py +++ b/traits/tests/test_observe.py @@ -52,7 +52,7 @@ def _do_something_when_foo_changes(self): pass with self.assertWarnsRegex(UserWarning, message_regex): - class A(HasTraits): + class B(HasTraits): foo = Int() @observe("foo") From ff673088c7baa2ad12337f8ea0ca4ffa97dbf9b9 Mon Sep 17 00:00:00 2001 From: Mark Dickinson Date: Wed, 15 Sep 2021 15:04:40 +0100 Subject: [PATCH 5/6] Two more valid method signatures --- traits/tests/test_observe.py | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/traits/tests/test_observe.py b/traits/tests/test_observe.py index f0883e274..0f8f01234 100644 --- a/traits/tests/test_observe.py +++ b/traits/tests/test_observe.py @@ -68,6 +68,10 @@ class A(HasTraits): call_count = Int(0) + @observe("foo") + def _the_usual_signature(self, event): + self.call_count += 1 + @observe("foo") def _method_with_extra_optional_args(self, event, frombicate=True): self.call_count += 1 @@ -80,10 +84,14 @@ def _method_with_star_args(self, *args): def _method_with_alternative_name(self, foo_change_event): self.call_count += 1 + @observe("foo") + def _optional_second_argument(self, event=None): + self.call_count += 1 + a = A() self.assertEqual(a.call_count, 0) a.foo = 23 - self.assertEqual(a.call_count, 3) + self.assertEqual(a.call_count, 5) class Student(HasTraits): From 7933f0b8fc2bb94f6bc27f6bbcb5356c7cd8500a Mon Sep 17 00:00:00 2001 From: Mark Dickinson Date: Thu, 16 Sep 2021 13:20:05 +0100 Subject: [PATCH 6/6] Remove unnecessary exception handling --- traits/has_traits.py | 30 +++++++++++++----------------- 1 file changed, 13 insertions(+), 17 deletions(-) diff --git a/traits/has_traits.py b/traits/has_traits.py index 6c9f5b586..883619b31 100644 --- a/traits/has_traits.py +++ b/traits/has_traits.py @@ -845,24 +845,20 @@ def observe_decorator(handler): # Warn on a dubious handler signature. The handler should accept a call # that passes a single positional argument (conventionally named # "event") in addition to the usual "self". + handler_signature = inspect.signature(handler) try: - handler_signature = inspect.signature(handler) - except (ValueError, TypeError): - pass - else: - try: - handler_signature.bind("self", "event") - except TypeError: - warnings.warn( - ( - "Dubious signature for observe-decorated method. " - "The decorated method should be callable with a " - "single positional argument in addition to 'self'. " - "Did you forget to add an 'event' parameter?" - ), - UserWarning, - stacklevel=2, - ) + handler_signature.bind("self", "event") + except TypeError: + warnings.warn( + ( + "Dubious signature for observe-decorated method. " + "The decorated method should be callable with a " + "single positional argument in addition to 'self'. " + "Did you forget to add an 'event' parameter?" + ), + UserWarning, + stacklevel=2, + ) try: observe_inputs = handler._observe_inputs