Generalize XMPP subprotocol containment

[ralphm]

In twisted.words.protocols.jabber.component an interface IService, and accompanying implementation Service are defined. Together with ServiceManager they allow for implementing a server side XMPP component as a set of subcomponents that each implement parts of the (sub)protocols the component understands.

Other XMPP entities (clients, servers) could also benefit from such an abstraction, so a more general form would be useful. Also, in relation to #1995, the question was raised if this should be derived from twisted.application.service.Service.

[glyph]

The description of this ticket says "subprotocol", but then the classes involved are named "service". Given the confusion with other interfaces and classes named "service", maybe it would better be named something like "Subprotocol"? :)

I don't think that subclassing from t.a.a.S is a good idea, because their purposes seem completely different.

[ralphm]

That was basically the point of the discussion. The way I use this functionality thought up by dizzyd doesn't really use the t.a.s.Service semantics.

What I do in my projects is split up the handling of the incoming and outgoing traffic over several objects. E.g. one that handles service discovery, one that handles presence and one that handles publish-subscribe. It is very nice to compose an XMPP entity in this way.

[ralphm]

I've been experimenting with an implementation as part of my work on  Mimír. It involves a StreamManager service that holds protocol handler objects. I called the interface IEnhancementProtocol, but I am contemplating IXMPPHandler.

The nice thing about this abstraction is that a StreamManager can be extended to something called XMPPClient or XMPPComponent.

I'll create a branch and move the experiment in there. It would be nice to have this in Words 0.5 (and therefore the Twisted 2.5 bundle?).

[ralphm]

(In [23198]) Branching to 'xmpp-subprotocols-2178-5'

[ralphm]

Finally submitting this for review, after a short excursion to  Wokkel, with more to move to Words shortly.

[therve]

• StreamManager should call XMPPHandlerCollection.__init__, and not duplicate attributes
• please use zope.interface.verify.verifyClass/verifyObject to check that the classes/objects correctly implements/provides the interfaces
• the test methods names with 2 '_' is ugly, one is enough.
• the logTraffic options of StreamManager is not tested
• I don't see the point of the manager attribute of IXMPPHandler. Isn't that redundant with the parent attribute?
• lots of undocumented methods in XMPPHandler. Generally it's just to explain why it's empty (not put redundant information with the interface)
• Isn't the send method part of the interface?
• 2 blanks lines between methods/3 blank lines at module level
• please update copyright notice

Thanks!

[ralphm]

Replying to therve:

* StreamManager should call XMPPHandlerCollection.__init__, and not duplicate attributes

Fixed.

* please use zope.interface.verify.verifyClass/verifyObject to check that the classes/objects correctly implements/provides the interfaces

Where should these be used exactly?

* the test methods names with 2 '_' is ugly, one is enough. * the logTraffic options of StreamManager is not tested

Fixed.

* I don't see the point of the manager attribute of IXMPPHandler. Isn't that redundant with the parent attribute?

The parent attribute was called manager in previous incarnations. I just forgot to change the interface. Fixed.

* lots of undocumented methods in XMPPHandler. Generally it's just to explain why it's empty (not put redundant information with the interface)

Should I just put a note to that effect in the class docstring?

* Isn't the send method part of the interface?

I wasn't sure if all IXMPPHandlers should have to implement this method. So for now, I didn't want to make it part of the interface. Also, this method is usually only called from within the class.

* 2 blanks lines between methods/3 blank lines at module level * please update copyright notice

Done.

Thanks!

Thank you!

[therve]

• You can see an example of verifyObject in twisted.test.test_process. It should be used in tests to check interfaces
• DummyFactory and DummyXMPPHandler are missing documentation.
• XMPPHandlerTest and StreamManagerTest are missing class docstrings
• I don't know what's the best strategy to document XMPPHandler, but it's clearly missing some. I think the empty methods should say 'Called when XXX. Override in subclasses'.
• In XMPPHandlerCollection addHandler and removeHandler, you still refer to the manager attribute
• the _initialized of XMPPHandlerCollection is not documented
• several undocumented method in StreamManager

Back to you.

[ralphm]

I addressed the above issues. Thanks for showing me verifyObject, I never used it before and found a small problem with attribute initialization. The references to the manager attribute are really gone now.

[therve]

(In [23337]) Some blank lines tweaks.

Refs #2178

[therve]

I've made a small cleanup on blank lines. This is good for me, please merge.

[ralphm]

(In [23376]) Add abstraction for implementing XMPP protocol extensions.

Author: ralphm Reviewer: therve Fixes #2178.

This abstraction allows for implementing (parts of) XMPP protocol extensions in self-contained handlers of which instances can be added to an object that acts as a manager for a particular XML stream.

[exarkun]

Milestone Words-8.0.0+1 deleted