Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
A simple plugin system #380
A simple plugin system #380
Changes from 8 commits
b7a3692
b52b92c
dda82ff
e4975ad
0149237
eb93e56
319b9e2
6c7f1f2
6e39c65
fe0f9d9
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Actually, I'm not sure whether the method should have been overwritten - presumably
Stream.test_double
already existed before the call toregister_plugin_entry_point
.In normal usage, the stubs always come first, since they happen at import time, and streamz should always be imported before derived classes.
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.
Yes, I guess this test actually doesn't test anything. I wanted to test that running
Stream.register_api
twice on the same class doesn't break anything, in case the implementation ofregister_api
changes. We can just check for this inregister_plugin_entry_point
.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.
The test is OK, but not comprehensive. The cases are:
In every case, calling the method should result in the same test class.
We could check for whether the name is being overwritten, but I don't think that's essential.
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.
The class is stored in
Stream.method.__wrapped__
, but I don't see how anything else but the test class can end up there in any of these cases.Btw, what should we do if a plugin wants to override a built-in method? On the one hand, this would allow people to create useful extensions of built-in functionality (my version of
partition
could be provided as a plugin long before it's merged into core), on the other — it could break things and lead to confusion.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.
Personally I would have the system balk at a built-in override. My hope would be that names are cheap, so adding more text onto a would be
partition
describing how it is different would add less burden than issues around whichpartition
was used. This could get particularly thorny as envs get updated, code that previously worked and relied on the built-in are now producing errors in subtle ways that could be difficult to find.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.
You mean we should have a
clobber=False
argument for the register methods?I wonder in that case, if there is a sane way to tell when an entrypoint and register_api refer to the same thing, which would not be a problem even without clobber.
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.
We can just check that
hasattr(Stream, entry_point.name)
. But it would have to happen during plugin load (so that we're not overwriting built-in methods with stubs) and so importing streamz with a bad plugin would result in an error. Or it could be a warning that we skipped an entrypoint because of name collision.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.
but this will fail when someone adds the entrypoint to a class that already has register_api, no?
Note @CJ-Wright , that the current implementation does allow you to use register_api to overwrite methods at will. Of course this is python, so we can't ever disable someone who wants to do that anyway.
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.
That's fair. To some extent this boils down to locality for me. If the author of the code is the user of the env then if they override things then they are in a better place to clean up. When installing 3rd party things that state may not hold as well.
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.
I agree. With
register_api
things are explicit and visible. A plugin is a black box.@martindurant please look at the last commit to see what I mean