-
Notifications
You must be signed in to change notification settings - Fork 55
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 depends_on with observe in properties #881
Conversation
@@ -28,7 +28,7 @@ class GUIApplicationAction(ListeningAction): | |||
|
|||
# 'ListeningAction' interface -------------------------------------------- | |||
|
|||
object = Property(depends_on="application") | |||
object = Property(observe="application") |
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.
flag (see other comment)
@@ -125,7 +125,7 @@ def _object_changed(self, old, new): | |||
for kind in ("enabled", "visible"): | |||
method = getattr(self, "_%s_update" % kind) | |||
name = getattr(self, "%s_name" % kind) | |||
if old: | |||
if old and old is not Undefined: |
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.
This change was a bit strange, but it is necessary because of https://github.com/enthought/pyface/pull/881/files#r571222562 and here:https://github.com/enthought/traits/blob/aece1feca33e51890f9d0054606a7a241cb2776e/traits/has_traits.py#L327 I believe.
This also made me think that maybe Undefined
should not evaluate to true in if statements like this, but I do not have enough context on its use to say this at all confidently (it just seems like it would be intuitive).
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.
it's worth bringing this up on the channel and hearing from the rest of the team.
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.
Suggestion from side-discussion: perhaps use if isinstance(old, HasTraits)
(and the equivalent for new
below).
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
This PR is part of the transition to depending on traits 6.2. This PR replaces all use of
depends_on
withobserve
in property definitions throughout the code base. I did so via VS code find an replace, but manually went through to confirm the changes/updated a couple occurrences where we observed containers and needed to use the updated traits mini language.