Opened 8 years ago

Closed 6 years ago

Last modified 6 years ago

#2178 enhancement closed fixed (fixed)

Generalize XMPP subprotocol containment

Reported by: ralphm Owned by:
Priority: high Milestone: Twisted-8.2
Component: words Keywords:
Cc: ralphm, glyph, therve Branch: branches/xmpp-subprotocols-2178-5
(diff, github, buildbot, log)
Author: ralphm Launchpad Bug:

Description

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.

Change History (15)

comment:1 Changed 8 years ago by 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.

comment:2 Changed 8 years ago by ralphm

  • Cc ralphm glyph added

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.

comment:3 Changed 8 years ago by ralphm

  • Milestone set to Words-0.5

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?).

comment:4 Changed 6 years ago by ralphm

  • author set to ralphm
  • Branch set to branches/xmpp-subprotocols-2178-5

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

comment:5 Changed 6 years ago by ralphm

  • Keywords review added
  • Owner ralphm deleted
  • Priority changed from normal to highest

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

comment:6 follow-up: Changed 6 years ago by therve

  • Keywords review removed
  • Owner set to ralphm
  • Priority changed from highest to high
  • 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!

comment:7 in reply to: ↑ 6 Changed 6 years ago by ralphm

  • Owner changed from ralphm to therve

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!

comment:8 Changed 6 years ago by ralphm

  • Cc therve added
  • Keywords review added

comment:9 Changed 6 years ago by therve

  • Keywords review removed
  • Owner changed from therve to ralphm
  • 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.

comment:10 Changed 6 years ago by ralphm

  • Keywords review added
  • Owner changed from ralphm to therve

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.

comment:11 Changed 6 years ago by therve

(In [23337]) Some blank lines tweaks.

Refs #2178

comment:12 Changed 6 years ago by therve

  • Keywords review removed
  • Owner changed from therve to ralphm

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

comment:13 Changed 6 years ago by ralphm

  • Resolution set to fixed
  • Status changed from new to closed

(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.

comment:14 Changed 6 years ago by exarkun

  • Milestone changed from Words-8.0.0+1 to Twisted-8.2

Milestone Words-8.0.0+1 deleted

comment:15 Changed 3 years ago by <automation>

  • Owner ralphm deleted
Note: See TracTickets for help on using tickets.