Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix an observe-decorated _name_changed method #1560

Merged
merged 3 commits into from
Oct 1, 2021

Conversation

mdickinson
Copy link
Member

@mdickinson mdickinson commented Sep 30, 2021

This PR changes the behaviour of a method that's decorated with observe but also has the magic _somename_changed naming.

Prior to this PR, such a method would be registered twice by the Traits machinery, and called once with a TraitChangeEvent and once with the new value of the trait:

>>> from traits.api import *
>>> class A(HasTraits):
...     foo = Int()
...     @observe("foo")
...     def _foo_changed(self, arg):
...         print(arg)
... 
>>> a = A()
>>> a.foo = 23  # Expect one call to `print`; get two calls.
23
TraitChangeEvent(object=<__main__.A object at 0x10ad14450>, name='foo', old=0, new=23)

With this PR, it will only be called once, with the TraitChangeEvent: the observe decorator supersedes and nullifies the magic naming. This is the same behaviour that's already present for on_trait_change:

>>> from traits.api import *
>>> class A(HasTraits):
...     foo = Int()
...     @observe("foo")
...     def _foo_changed(self, arg):
...         print(arg)
... 
>>> a = A()
>>> a.foo = 23  # Get one call to `print`, as expected.
TraitChangeEvent(object=<__main__.A object at 0x1056994a0>, name='foo', old=0, new=23)

I don't think we need a deprecation period for the change in behaviour: I doubt that there's working code that relies on the method being called twice with different arguments for each call, so I think this qualifies as a bugfix.

The PR also adds tests that exercise the corresponding code for on_trait_change.

Closes #527

@mdickinson

This comment has been minimized.

Copy link
Member

@jwiggins jwiggins left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

traits/tests/test_has_traits.py Show resolved Hide resolved
@mdickinson mdickinson merged commit 2dcf651 into main Oct 1, 2021
@mdickinson mdickinson deleted the fix/decorated-changed-method branch October 1, 2021 10:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Decorating a static listener with @on_trait_change causes problems
2 participants