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

Rewrite "_*_changed" static trait handler methods to use "observe" #401

Merged
merged 4 commits into from
May 11, 2021

Conversation

rahulporuri
Copy link
Contributor

This PR updates most, but not all, of the _*_changed static trait handler methods to use traits observe instead. See related PR #400

Poruri Sai Rahul added 2 commits April 16, 2021 10:04
This commit updates most, but not all, of the _*_changed trait handler
methods to use observe instead

	modified:   envisage/application.py
	modified:   envisage/composite_plugin_manager.py
	modified:   envisage/core_plugin.py
	modified:   envisage/egg_basket_plugin_manager.py
	modified:   envisage/package_plugin_manager.py
	modified:   envisage/plugin_extension_registry.py
	modified:   envisage/plugin_manager.py
	modified:   envisage/plugins/text_editor/editor/text_editor.py
	modified:   envisage/tests/event_tracker.py
	modified:   envisage/tests/test_plugin.py
	modified:   envisage/ui/action/abstract_action_manager_builder.py
	modified:   envisage/ui/tasks/action/task_window_launch_group.py
	modified:   examples/legacy/plugins/tasks/ipython_kernel/python_editor.py
	modified:   examples/legacy/plugins/workbench/Lorenz/acme/lorenz/lorenz.py
	modified:   envisage/composite_plugin_manager.py
Copy link
Contributor

@aaronayres35 aaronayres35 left a comment

Choose a reason for hiding this comment

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

LGTM

Just a few comments

envisage/plugin_manager.py Outdated Show resolved Hide resolved
envisage/plugins/text_editor/editor/text_editor.py Outdated Show resolved Hide resolved
envisage/tests/event_tracker.py Outdated Show resolved Hide resolved
Poruri Sai Rahul and others added 2 commits May 11, 2021 08:51
the method names will hopefully better convey what the methods will do -
instead of conveying what the methods are reacting to

	modified:   envisage/plugin_manager.py
	modified:   envisage/plugins/text_editor/editor/text_editor.py
	modified:   envisage/tests/event_tracker.py
@rahulporuri rahulporuri requested a review from aaronayres35 May 11, 2021 10:29
@rahulporuri
Copy link
Contributor Author

@aaronayres35 can you take another look at this PR?

Copy link
Contributor

@aaronayres35 aaronayres35 left a comment

Choose a reason for hiding this comment

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

Still LGTM, I have one more possible name suggestion but don't hesitate to ignore it

:shipit:


def __plugins_items_changed(self, trait_name, old, new):
@observe("_plugins:items")
def _update_application_on_changed_plugins(self, event):
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: maybe _update_application_on_plugins_contents?
I'm not sure thats really any clearer though. Feel free to ignore

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i can't think of a better name but i don't think _update_application_on_plugins_contents is better 😅 sorry. im going to merge for now.

@rahulporuri rahulporuri merged commit 28f7958 into master May 11, 2021
@rahulporuri rahulporuri deleted the dev/rewrite-trait-changed-magic-methods branch May 11, 2021 12:25
This was referenced Jun 8, 2021
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