Create guide on how to create plugins #1429
Conversation
cburgdorf
force-pushed
the
christoph/docs/plugin-guide
branch
2 times, most recently
from
a5197a4
to
1e5e3ce
Compare
| different for a :class:`~trinity.extensibility.plugin.BaseMainProcessPlugin` but fairly similar | ||
| for the other types of plugins which we'll be focussing on for now. | ||
|
|
||
| Plugins need to implement the :meth:`~trinity.extensibility.plugin.BasePlugin._start` method |
There was a problem hiding this comment.
@pipermerriam remember when we talked about the _start / start pair where start is calling _start and _start is meant to be implemented whereas start is meant to be called because it does some work and then delegates to _start behind the scenes. #1331 (comment)
The naming of _start is proving to be problematic from a documentation standpoint because it is considered a private API and those are by default not being considered for the docs (rightfully so). This also means that linking to them won't work.
I wonder if we can find a generic pattern for this where we derive a name similar to the public API name but without making it private. I think this is not only good from a documentation standpoint but it also strikes me as the right thing to do. We use the same pattern in the BaseService so it would be nice if we can find a generic naming convention. E.g.
start -> do_start
stop -> do_stop
...
I'm usually bad at naming but I kinda like the do prefix. Thoughts? /cc @carver
There was a problem hiding this comment.
Btw, no need to review the rest of the PR. I just put it out for transparency but the whole text, structure is still very rough, incomplete and in flux.
There was a problem hiding this comment.
do_start seems fine. You could also invert these and make _start the one that is called internally and start the one that the user implements.
There was a problem hiding this comment.
You could also invert these
That would work for the implementation as of today where Plugins call start themselves but I would still consider both APIs public. It is more clear for the stop API that is called from the PluginManager hence it is definitely public.
But if you are ok with the do prefix then I'll send a PR for that. Thinking about it, it was strange to have a method that is intended to be overwritten to be private anyway. That wouldn't even work in most languages (e.g. Java, C# etc)
There was a problem hiding this comment.
@cburgdorf as stated in the other PR, I'm fine with the do_ prefix on the plugin API but I'm 👎 on immediately extending it to the rest of the codebase.
There was a problem hiding this comment.
but I'm -1 on immediately extending it to the rest of the codebase.
@pipermerriam is this because we need to find a better prefix than do? Because, from a language theory it strikes me as plain wrong that we have something like BaseService._run as private and abstract at the same time. In fact, I don't know any statically typed language (which Python isn't but there more we use mypy the more we try to be haha) that would allow this.
In something like Java or C#, if you want subclasses to be able to overwrite a method, or even force them to, then that method can not be private.
And it has real consequences, it means that if we document the BaseService like this:
.. autoclass:: p2p.service.BaseService
:members:
Then, _run and _cleanup, probably the most important methods of this class, those that need to be overwritten, simply fall out of the documentation because they are incorrectly seen as private. Something that would be impossible in most if not all statically typed language that have the concepts of private members and inheritance.
There was a problem hiding this comment.
Btw, I would be seam totally reasonable to me if in the future mypy would raise warnings or even errors for when private members are overwritten in subclasses. On the naming part, here are two alternatives for prefixes:
protected_(borrowed from the access modifier thatJava,C#andTypeScriptuse for members that are accessible in subclasses but not from other classes which kinda goes nicely hand in hand with our intend)sealed_just because it is a little shorter and seems to convey the same as the former
There was a problem hiding this comment.
I think what we're struggling with here is that a bad architectural design is triggering a negative side-effect of some weird OOP practices (this private/protected method that must be implemented by subclasses).
Two types of needs call for two types of solutions:
- Urgent feature: patch the side effect by adding
do_(I think Christoph feels urgency on the plugin docs, so they are in this category) - Easy maintenance: attack the root architectural problem, probably with converting inheritance to compositional approach. (I think
BaseServiceand other instances in the code base
are in this category)
There was a problem hiding this comment.
Added my answer in the PR. #1440 (comment)
cburgdorf
force-pushed
the
christoph/docs/plugin-guide
branch
from
1e5e3ce
to
ff52f65
Compare
These methods were named so that they appeared private by convention. This convention causes trouble in the documentation where these methods are righfully dropped. Since these methods aren't really private in reality (they need to be overwritten in subclasses), they were renamed, migrating their naming from `_*` to `do_*`.
This enables the plugin to read the status of the four formal states a plugin can be in. Previously even though the four states existed implicitly, it could only be differentiated between the STARTED/STOPPED state based on the `running` bool. This change also has the nice benefit that we can make stronger assumptions about the status inside `start()` and raise an exception if `start()` was called on a plugin that isn't in the correct status to call `start()`
cburgdorf
force-pushed
the
christoph/docs/plugin-guide
branch
3 times, most recently
from
6299f3a
to
b7168be
Compare
Relates to ethereum#1103
cburgdorf
force-pushed
the
christoph/docs/plugin-guide
branch
from
88e75f7
to
f4184a5
Compare
|
Merged this without review as discussed with @pipermerriam in the chat to get the draft guide up online for DEVCON IV. |
What was wrong?
For people to start contributing Trinity plugins or building their own applications with the Trinity + Plugins Model we need to do a good job of teaching them how to use the Plugin API. So we need a detailed guide for the Plugin API. Notice that this PR is a spin off from #1424 to allow #1424 to get merged sooner while I take more time for writing the guide.
How was it fixed?
This is a draft version that we may merge anyway to have something for DEVCON !V
Cute Animal Picture