Skip to content
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 on_trait_change with observe #285

Merged
merged 10 commits into from
Apr 1, 2021

Conversation

aaronayres35
Copy link
Contributor

@aaronayres35 aaronayres35 commented Mar 30, 2021

Checklist

  • Add a news fragment if this PR is news-worthy for end users. (see docs/releases/README.rst)

This PR replaces almost all uses of on_trait_change with observe.

All that remains is apptools.selection.selection_service.py and apptools.scripting.recorder.py (also an undo example but that is deprecated here anyway). These are a bit trickier and can be addressed separately

@rahulporuri rahulporuri self-requested a review March 31, 2021 03:57
Copy link
Contributor

@rahulporuri rahulporuri left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM with a few comments/suggestions.

apptools/logger/plugin/logger_service.py Outdated Show resolved Hide resolved
@@ -121,7 +121,7 @@ def _initialize(self):
""" Wire-up trait change handlers etc. """

# Listen for the object's trait being changed.
self.obj.on_trait_change(self._on_trait_changed, self.trait_name)
self.obj.observe(self._on_trait_changed, self.trait_name)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks like we don't have a good place to unhook this listener

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah I noticed this as well... not sure what the best thing to do is

apptools/preferences/ui/widget_editor.py Outdated Show resolved Hide resolved
@aaronayres35
Copy link
Contributor Author

Orthogonal to this PR, but we should add a "maintenance" news fragment type. I wasn't sure which of "feature, bugfix, deprecation, removal, doc, test, build" to classify this PR as, but it feels like it will be ultimately relevant for the changelog (as I imagine 5.1 of apptools main purpose will be this migration to observe / depending on traits 6.2)

I am going to call it a feature for now simply because IDK where to put it. When generating the actual changelog we will want to move this, but this way it will still be picked up

@aaronayres35 aaronayres35 merged commit 45d19d0 into master Apr 1, 2021
@aaronayres35 aaronayres35 deleted the replace-on_trait_change-with-observe branch April 1, 2021 19:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants