always perform requested trait assignments #2311
Conversation
|
We're unlikely to break it again, but do you think we should have a test for that? Also, some unrelated javascript changes have appeared - going by the commit message, I guess they're not meant to be here. |
|
oops, js is just me failing to properly stash what I was fiddling with at the time, I will remove it. I will add a test, might as well. |
Some assignments do not change *value*, but do change the object, and these should not be ignored. For instance, container objects are currently impossible to assign if they evaluate as equal.
|
test added |
|
Test results for commit 58fa955 merged into master (289fc01)
Not available for testing: |
| if old_value != new_value: | ||
| obj._trait_values[self.name] = new_value | ||
| obj._notify_trait(self.name, old_value, new_value) |
There was a problem hiding this comment.
We still only do the change notification if the new value doesn't compare equal. Does that make sense? Do we have any traits for mutable types that trigger notifications?
There was a problem hiding this comment.
There are a number of situations where it helps end notification cycles. traits proper lets you configure this on a per-trait basis, but this is not used especially often.
There was a problem hiding this comment.
I could change it to an is check.
There was a problem hiding this comment.
An is check intuitively makes more sense to me, but I haven't looked for
possible side effects.
There was a problem hiding this comment.
I just worry about infinite loops with an is check, in case there is some
casting operator involved in the trait.
There was a problem hiding this comment.
With an is check, infinite recursion is easy to trigger with a copy:
class C(HasTraits):
d = Dict()
def _d_changed(self, name, old, new):
self.d = new.copy()So that doesn't seem like a good choice. Maybe it should be left as it is?
There was a problem hiding this comment.
OK, that's fine, I just wanted to make sure we'd thought about what API
we'd expect, as this creates the possibility that a trait can be changed
without the notifications being fired.
|
@minrk I think you can merge this unless you want to leave it for later. |
always perform requested trait assignments Some assignments do not change value, but do change the object, and these should not be ignored. For instance, container objects are currently impossible to assign if they evaluate as equal, even if their types don't match (e.g. assigning an empty defaultdict to a Dict trait, which prompted this PR).
always perform requested trait assignments Some assignments do not change value, but do change the object, and these should not be ignored. For instance, container objects are currently impossible to assign if they evaluate as equal, even if their types don't match (e.g. assigning an empty defaultdict to a Dict trait, which prompted this PR).
Some assignments do not change value, but do change the object,
and these should not be ignored.
For instance, container objects are currently impossible to assign
if they evaluate as equal, even if their types don't match
(e.g. assigning an empty defaultdict to a Dict trait)