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

Propose new plugin interface #248

Closed
wants to merge 1 commit into from

Conversation

dbluhm
Copy link
Member

@dbluhm dbluhm commented Nov 4, 2019

I'd like to propose a slightly different mechanism for loading protocol handlers as well as other features. Rather than specifically looking for message types inside of a loaded module, I propose that we attempt to load and execute an asynchronous setup method, with the context passed as the first argument. This would allow side loaded modules to not only add protocol handlers but also potentially define new ledgers, admin servers, admin server routes, etc. without having to fork the ACA-Py code base. The changes I've included here are mostly just a proof of concept of what this might look like and other changes are likely necessary to fully support side loading something like a ledger.

More than open to thoughts and feedback, why this approach is a bad idea, ideas for a better approach etc.

Signed-off-by: Daniel Bluhm <[email protected]>
@codecov-io
Copy link

Codecov Report

Merging #248 into master will decrease coverage by 0.03%.
The diff coverage is 33.33%.

@@            Coverage Diff             @@
##           master     #248      +/-   ##
==========================================
- Coverage   74.53%   74.49%   -0.04%     
==========================================
  Files         219      219              
  Lines       10162    10171       +9     
==========================================
+ Hits         7574     7577       +3     
- Misses       2588     2594       +6

@andrewwhitehead
Copy link
Member

I feel like this should replace --protocol, not complement it

@dbluhm
Copy link
Member Author

dbluhm commented Nov 4, 2019

I feel like this should replace --protocol, not complement it

That sounds reasonable to me; my thoughts with leaving it in was not breaking compatibility for anyone currently using the --protocol argument.

@andrewwhitehead
Copy link
Member

@dbluhm I think we'll add this change as part of some other refactoring we're doing at the moment, but I'm looking at loading all the plugin modules into the ProtocolRegistry early on, then calling the setup method for each one (if defined) when the context is initialized. Admin routes would be loaded based on the same module registry.

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.

4 participants