-
Notifications
You must be signed in to change notification settings - Fork 98
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
Replace some _anytrait_changed methods #625
Conversation
The occurrence in However, this may lead to some confusion. Additionally, the metadata use in |
looks like the intermittent failure on On linux/pyqt
|
chaco/axis.py
Outdated
] | ||
if name in invalidate_traits: | ||
self._invalidate() | ||
@observe("+is_visual") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I prefer listing out the various traits that need to be observed here - instead of introducing new trait metadata. If we can make do without adding anything new, we should persue that option imo. I feel like introducing new trait metadata introduces a new maintenance/cognitive overhead.
"max_y", | ||
]: | ||
self.compute_model() | ||
@observe("+needs_compute") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same comment as before - i'd prefer verbosely listing out all of the traits that we want to listen to - instead of introducing new trait metadata.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
closes #624
Note there are still a few examples using
_anytrait_changed
methods that can be addressed in a different PR. I ended up not using metadata in thelegend.py
and just went with the straight forward conversion to observe. We can come back to this if we want to be a little more clever later.