Rename _start/_stop/_run_/_cleanup to use do_ prefix

[cburgdorf]

What was wrong?

We had a bunch of methods that we called _<something> not because they are considered private but mainly because <something> already exist where <something> calls _<something> behind the scenes (after performing other work).

These methods are meant to be overwritten and hence in the classical sense of OOP can not be considered private anyway (e.g. could not be private in most statically typed languages).

Also this has proven to be problematic for the documentation because private methods are by default not included in the generated docs (righfully so).

How was it fixed?

Changed _ prefix to do_ prefix

Cute Animal Picture

[cburgdorf]

@pipermerriam I think elaborating on this pattern and settling on the do_ naming convention would probably also be a good fit for our tactical manual. Do you agree?

[cburgdorf]

Ooops

[pipermerriam]

Conceptually 👍 but I'm not sure I love the do_ prefix. Can we invert this model and have:

• start - public user facing API?
• _start - internal API?

such that _start calls start (where-as it is currently the other way around).

[cburgdorf]

No matter how you turn it, you want to have a method in the base class (BasePlugin) that is called from another class (the derived plugins), and you want to have an abstract class in the base class that derived classes need to overwrite. That makes both methods non-private by regular OOP standards (as in: you couldn't even do it with private methods in statically typed languages such as C#, Java)

And we also want to document both methods. We want to document that there is start that a plugin needs to call to start and there is do_start that needs to be implemented by plugins to provide the starting logic. Non of this is private and we would need to apply some hack to get API docs for this if we keep one of them private.

And if you step away from this particular case, it becomes even clearer. You want the BasePlugin to define an abstract method for derived classes to be implemented (do_stop) and you want the base class to have a stop method (that will then delegate to do_stop) that needs to be called from within the PluginManager.

So both methods need to be non-private, and we also want to document both.

[carver]

I'm currently 👎 on the change, because it feels like we're patching up a problem rather than attacking it at it's root. If patching helps accomplish some urgent goal, I'm good with it. But it seems like the motivation here is to find our universal pattern for solving this problem, and I don't think this is it. (I guess I'm just restating: #1429 (comment) )

---

No matter how you turn it, you want to have a method in the base class (BasePlugin) that is called from another class (the derived plugins), and you want to have an abstract class in the base class that derived classes need to overwrite.

This is an assumption I would want us to question, when trying to find our pattern. If you look at a service subclass (say Peer) as its own entity, we get a mutual dependence on the superclass in the current design.

BaseService <--> Peer

• Peer depends on BaseService attrs like is_operational
• BaseService depends on Peer attrs like _run
• Both can reach into the private attrs of each other

---

An example solution might look like breaking the two entities into a few pieces:

AbstractService
  ^           ^
  |            \
ServiceMixin -> Service -> AbstractServiceSpec
  ^
  |
Peer -> PeerServiceSpec -> AbstractService

Roughly:

• AbstractService would have the public API methods of service, like run(), run_child_service() cancel()
• ServiceMixin would re-expose only some of the Service methods, like run() and cancel().
• Service would be initialized with an implementation of an AbstractServiceSpec, which exposes a main_loop() (formerly _run()) and cleanup() (formerly _cleanup())
• PeerServiceSpec actually implements main_loop() and cleanup(), which probably requires using service APIs like run_child_service()
• Peer would implement public API methods like send() that are independent from the service API

I'm certain there are problems with this design, otherwise I'd be pitching to use it now. I think it would take a little while to work out the full solution. It's just a starting point for discussion, and an example of attacking the root problem. (An example problem I think there is an initialization cycle in Service->PeerServiceSpec->Service, even though the dependencies are not mutual anymore).

[cburgdorf]

@carver I agree that there may be a solution that addresses the problem at the architectural level and your proposed solution may just be that.

However, to me, it feels as if our CI just broke because mypy told us we are overwriting private methods in a parent class which the type system should forbid. Just that it wasn't mypy that discovered this but sphinx.

So just renaming _* to do_* for the affected cases strikes me as a reasonable, fast answer to ensure we aren't doing things that the types system should prevent us from doing (and that future mypy versions may end up preventing).

However, I don't wanna be stubborn about it. If you and @pipermerriam are 👎 on this quick fix, I'll convert this PR into an issue, restating the problem and its consequences so that we can address it with a different fix later.

For the affected plugin code I will update _* to do_* for the affected places based on your both agreement to use this mitigation there.

[pipermerriam]

I'm good with a stop-gap fix for anything with an immediate need. I think @carver analysis is probably a good starting point for someone to spin out a secondary implementation of the Service class and to see if it works out (after which we can iteratively migrate everything over to this new paradigm.