-
Notifications
You must be signed in to change notification settings - Fork 44
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 dynamic uses of on_trait_change with observe part 2 #663
Conversation
@@ -93,7 +93,7 @@ def prepare(self, parent): | |||
""" | |||
name = self.extended_name |
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.
BEWARE: extended_name
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.
Meaning what, exactly? We can't be sure that name
won't break when passed to observe
?
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.
Yeah, a quick search of the code base for extended_name
only gives this result here and the corresponding same thing in the wx file. Any downstream users that set this trait may run into problems if the extended name they are passing does not conform to the new mini language, and that is effectively out of our hands ...
A similar scenario was encountered in pyface and we mentioned it in the release announcement. I imagine the vast majority of users would not be affected (I dont even know how many users of this code we have in total). Nonetheless, it leaves a door open for potential breaks.
Another thing to worry about here though that I'm not sure what we would want to do for: The traitsUI code for Editor
contains the code that would theoretically remove this listener (in either the dispose
method or the _update_editor
method itself. I also just realize I made a dumb mistake! The method being observed is _update_editor
not update_editor
which is defined in traitsUI. We are still in the process of updating traitsUI to observe, so that method will need its signature changed... I believe we will need that around first, before we can make this change as this code won't work currently. It must be uncovered in the test suite hence we don't see a failure here
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. The test for AbstractWindow
looks fine.
part of #416
This PR finishes the job started in #656 by replacing the remaining dynamic uses of
on_trait_change
withobserve
.