Fix repeated Property triggering in subclasses #1587
Conversation
| @@ -559,8 +559,7 @@ def update_traits_class_dict(class_name, bases, class_dict): | |||
|
|
|||
| observer_states = getattr(value, "_observe_inputs", None) | |||
| if observer_states is not None: | |||
| stack = observers.setdefault(name, []) | |||
| stack.extend(observer_states) | |||
| observers[name] = observer_states | |||
There was a problem hiding this comment.
This is a minor consistency cleanup that should not affect behaviour: observer was created fresh as an empty dictionary ahead of the for loop containing this code; assignment to any given name can only occur once for that name during execution of the for loop.
There was a problem hiding this comment.
do you mean observers - not observer - the empty dictionary?
There was a problem hiding this comment.
Sorry, yes: observers.
| if name not in class_traits and name not in class_dict: | ||
| stack = observers.setdefault(name, []) | ||
| stack.extend(states) | ||
| if (name not in class_traits) and (name not in class_dict): |
There was a problem hiding this comment.
Similarly, this should not affect behaviour: the only way for name to already be present in observers is for it to have been added by the for loop that processes class_dict, but in that case the name not in class_dict condition will fail and this code will not be reached.
To double check the reasoning, I added an assert name not in observers to the old code and ran the entire test suite; that assert wasn't triggered.
There was a problem hiding this comment.
Hmm. Looking more closely at this, there is actually a behaviour change here: the old code makes a (shallow) copy of the observer states, while this code just refers to the same list.
I could change this to states.copy(), but I don't think that change is necessary: I'm fairly sure that we don't mutate the states anywhere.
There was a problem hiding this comment.
we don't mutate the states anywhere
That should say: "we don't mutate the list of states anywhere". (I don't think we mutate the state dictionaries themselves either, but that's not relevant here.)
| @@ -758,8 +756,7 @@ def update_traits_class_dict(class_name, bases, class_dict): | |||
| property_name=name, | |||
| cached=trait.cached, | |||
| ) | |||
| stack = observers.setdefault(name, []) | |||
| stack.append(observer_state) | |||
| observers[name] = [observer_state] | |||
There was a problem hiding this comment.
This change is the one that introduces a behaviour change and fixes the issue.
| @@ -559,8 +559,7 @@ def update_traits_class_dict(class_name, bases, class_dict): | |||
|
|
|||
| observer_states = getattr(value, "_observe_inputs", None) | |||
| if observer_states is not None: | |||
| stack = observers.setdefault(name, []) | |||
| stack.extend(observer_states) | |||
| observers[name] = observer_states | |||
There was a problem hiding this comment.
do you mean observers - not observer - the empty dictionary?
(cherry picked from commit c903296)
|
#1588 is a backport of this PR to the 6.3 maintenance branch. |
This PR is a minimal, non-invasive fix for #1586, suitable for inclusion in a bugfix release. I (just barely) resisted the temptation to do a more significant refactoring of
update_traits_class_dict.Explanation of the fix:
update_traits_class_dictis invoked by theMetaHasTraitsmachinery for eachHasTraitssubclass that's declared. It roughly proceeds in three stages:HasTraitssubclass being built, looking for trait definitions,Propertydefinitions, andon_trait_changeandobserve-decorated methodsdepends_onandobservemetadataIn the situation of #1586, when processing
TestSubclass, stage 2 brought in thefoolistener for the base class'sProperty, and then stage 3 added another, identical listener.This PR fixes the logic to match what's done for
on_trait_change: there, stage 2 brings in the base class listener, and then stage 3 overwrites that listener with a new one. Forobserve, stage 3 is extending rather than overwriting, and this PR changes the logic so that it overwrites.Finally, this PR also makes two non-behavioural changes for consistency and clarity: the fix above introduces the situation that we're sometimes extending the list of observers for a given name and sometimes overwriting. But in both stage 1 and stage 2 I believe it's the case that the list of observers being extended must be the empty list, so we can replace those with a simple assignment.
I'm failing to find a use-case for the original logic that extended the list of observers.
Fixes #1586.