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

Plugins: Relocate plugin activate() method to Activator class, add simpler search method #3720

Merged
merged 6 commits into from Feb 6, 2019

Conversation

Projects
None yet
5 participants
@clintoncwolfe
Copy link
Contributor

clintoncwolfe commented Jan 12, 2019

This pays some technical debt from the early days of writing the plugin system. At first, Activators were bare structs, containers for data with no behavior. It was the registry's job to find activators, and then activate them.

Over time, those two operations fell into a clear pattern: search the registry for an activator using certain criteria, take the first match, and then pass its ids to Registry to have it activated. That was silly, since you already had an Activator object as the result of the first search.

This PR:

  • Puts the activate() method in Activator where it belongs
  • Adds a convenience method to Registry which searches for Activators with the assumption you expect exactly one match.

@clintoncwolfe clintoncwolfe force-pushed the cw/plugins-move-activate-method branch from 3564af4 to 9acd94a Jan 28, 2019

@clintoncwolfe clintoncwolfe requested a review from miah Jan 28, 2019

@miah

miah approved these changes Jan 29, 2019

Copy link
Contributor

miah left a comment

Thanks for paying down our debt @clintoncwolfe!

@robbkidd

This comment has been minimized.

Copy link
Contributor

robbkidd commented Jan 31, 2019

All in all, huge fan of plugins/activators knowing how to register/activate themselves. 👍

huge fan

clintoncwolfe added some commits Jan 12, 2019

Move plugin activate() method into Activator class
Signed-off-by: Clinton Wolfe <clintoncwolfe@gmail.com>
Convenience method for the common case of finding exactly one plugin …
…activator

Signed-off-by: Clinton Wolfe <clintoncwolfe@gmail.com>
linting
Signed-off-by: Clinton Wolfe <clintoncwolfe@gmail.com>
Update activate() calls used by DSL plugin type
Signed-off-by: Clinton Wolfe <clintoncwolfe@gmail.com>
Remove quotes from symbol representation of activated?
Signed-off-by: Clinton Wolfe <clintoncwolfe@gmail.com>
Remove spurious 'unless activated?' on most activation calls
Signed-off-by: Clinton Wolfe <clintoncwolfe@gmail.com>

@clintoncwolfe clintoncwolfe force-pushed the cw/plugins-move-activate-method branch from d2c4d98 to 591317e Feb 6, 2019

@jerryaldrichiii
Copy link
Contributor

jerryaldrichiii left a comment

Looks good to me! Thanks @clintoncwolfe!

@clintoncwolfe clintoncwolfe merged commit 14ef38f into master Feb 6, 2019

4 checks passed

DCO This commit has a DCO Signed-off-by
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
expeditor/config-validation Validated your Expeditor config file
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment