Opened 10 years ago

Closed 10 years ago

#5273 enhancement closed fixed (fixed)

Improve the API documentation for MultiService.addService to clarify its use in comparison to Service.setServiceParent

Reported by: David Kao Owned by: Jean-Paul Calderone
Priority: normal Milestone:
Component: core Keywords: documentation
Cc: David Kao Branch:
Author:

Description

Let's say

child = twisted.application.service.Service()

parent = twisted.application.service.MultiService()

child.setServiceParent(parent)

vs.

parent.addService(child)

behave differently.

The implementation of MultiService's addService does NOT make itself as the parent of child.

Meanwhile, the child, twisted.application.service.Service's setServiceParent not only adds itself to the parent via the call "addService" on the parent, it also sets its own parent attribute.

twisted.application.service.MultiService implements IServiceCollection, whose documentation says "Add a child service" for addService.

The behavior between MultiService's addService and a the Service's setServiceParent are asymmetric.

And it feels like twisted had intended to organize all services hierarchy in a tree.

If this is intended please tell me why and doc it so that ppl like me don't keep asking about it. Thanks!

Attachments (1)

addservice-doc.patch (2.5 KB) - added by Jean-Paul Calderone 10 years ago.

Download all attachments as: .zip

Change History (7)

comment:1 Changed 10 years ago by David Kao

Cc: David Kao added

Changed 10 years ago by Jean-Paul Calderone

Attachment: addservice-doc.patch added

comment:2 Changed 10 years ago by Jean-Paul Calderone

Keywords: review added

comment:3 Changed 10 years ago by Jean-Paul Calderone

Keywords: documentation added
Summary: MultiService's addService should make itself parent of child.Improve the API documentation for MultiService.addService to clarify its use in comparison to Service.setServiceParent
Type: defectenhancement

comment:4 Changed 10 years ago by indigo

Keywords: review removed
Owner: set to Jean-Paul Calderone

If we take the howto documentation an an example, it only ever uses setServiceParent; it never uses addService. It doesn't come right out and say it, but I had inferred that setServiceParent is the right thing to do as a user of the application framework, while addService is only relevant if one is implementing an IServiceCollection.

I'd say the API documentation should be more explicit in discouraging the use of addService directly. Besides the issues you mentioned, addService appears to perform no check that the child being added does not already have a parent. setServiceParent, on the other hand, will disown the present parent if there is one, thus guaranteeing that the services remain a non-overlapping hierarchy.

I'm not sure if being a non-overlapping hierarchy is really a requirement, but in the absence of anything explicitly to the contrary, it's reasonable to assume it is. If I'm wrong, then that possibility (of being an overlapping hierarchy) should be more explicit.

comment:5 Changed 10 years ago by indigo

I guess I missed the part about "Only implementations of L{IService.setServiceParent} should use this method". Whoops. Looks good to merge.

comment:6 Changed 10 years ago by Jean-Paul Calderone

Resolution: fixed
Status: newclosed

(In [32713]) Improve IServiceCollection.addService API docs, clarifying when the method is useful

Author: exarkun Reviewer: indigo Fixes: #5273

IServiceCollection.addService is really an aid to IService.setServiceParent implementations; point this out in the API documentation, so users know which one they should be calling themselves to construct the service hierarchy.

Note: See TracTickets for help on using tickets.