Opened 15 years ago

Closed 15 years ago

#1995 enhancement closed fixed (fixed)

Rework t.w.p.jabber.component to work with initializers.

Reported by: Ralph Meijer Owned by:
Priority: highest Milestone: Words-0.5
Component: words Keywords:
Cc: Ralph Meijer, Glyph, Jean-Paul Calderone Branch:
Author:

Description

After work on #1046 has been finished, the handshaking for server components can also be converted to use the new initializer concept.

Attachments (1)

handshake_as_initilizer.patch (3.1 KB) - added by Ralph Meijer 15 years ago.

Download all attachments as: .zip

Change History (12)

Changed 15 years ago by Ralph Meijer

comment:1 Changed 15 years ago by Ralph Meijer

Cc: Ralph Meijer added
Keywords: review added
Owner: Ralph Meijer deleted

Applied patch and added doctests in [18388]. Please review.

comment:2 Changed 15 years ago by Ralph Meijer

Priority: normalhighest

For completeness: source:branches/jabber-component-initializers-1995

comment:3 Changed 15 years ago by Glyph

Owner: set to Glyph
Status: newassigned

comment:4 Changed 15 years ago by Glyph

Keywords: review removed
Owner: changed from Glyph to Ralph Meijer
Status: assignednew

It says "added doctests" in your comment, but I don't see doctests, I see docstrings. That's good, though, Twisted's testing standard is not doctests, it's unit tests. Unfortunately I don't see any of those, either. Please write a few to cover the new functionality and re-review.

It might be worth mentioning here that it's strongly suggested to develop new code for Twisted test-first, if you can. In many situations that's really hard, but it seems like you know the domain pretty well here, so in the future try to write the tests before the implementation (it's a lot easier to see the coverage).

The module docstring should really contain at least a brief explanation (and a pointer to a longer one, if there is one) of what the word "external" and "component" mean. This is especially important because other areas of Twisted use these same words to mean other things: "external" meaning "not within the Twisted codebase" and "component" to mean "composed object implementing a Zope-style interface".

Some parameter names are changed here from "xmlstream" to "xs". This seems less clear, and is especially problematic since there is no @param in the appropriate docstrings explaining what the parameter was for. Call the parameter whatever you like as long as you have some documentation for it :).

The new docstring for "send" doesn't describe what "obj" is, and in my experience parameters with names like "obj" or "thunk" or "thing" are the ones that most desperately need clear documentation. What kind of object is "obj"?

Not a comment on this branch in particular, but there are a few problems with IService:

  • Its docstring doesn't explain its role or intended usage.
  • Its name conflicts with twisted.application.service.IService, which is an extremely common interface within Twisted.
  • It has only one implementor so far. There should be at least an example of a second implementation if it is an interface that users are supposed to use.

There's also a convention in the Divmod repository which we may want to adopt, to put all new interfaces into special "interfaces only" modules, so they can be imported without sucking in the whole implementation.

comment:5 Changed 15 years ago by Ralph Meijer

Cc: Glyph added

Sure, I meant docstrings. There is no new functionality, I just redid the handshake stuff such that it uses the new initializer stuff. The existing test ComponentAuthTest in test_jabbercomponent.py covers it.

I'll add some text on external components.

I changed xmlstream to xs, because xmlstream overrides an import and I use xs througout recently touched code. I'll add docstrings to compensate.

On send, I'll to the docstrings.

On IService, the interface is there because ServerManager invokes methods on its child services that implement this interface. I'll clarify this in the docstring. About the name, do you have a suggestion for a better name? Also, is component.Service, the only implementation of component.IService also a less fortunate name?

I would be ok with creating a interfaces.py, would it also go in t.w.p.jabber?

comment:6 Changed 15 years ago by Ralph Meijer

Keywords: review added
Owner: Ralph Meijer deleted

Improved docstrings.

glyph: in the future I might even spin out the Service and IService for use with similar ServiceManagers for server-to-server or client-to-server connections.

comment:7 Changed 15 years ago by Jean-Paul Calderone

Owner: set to Jean-Paul Calderone
Status: newassigned

comment:8 Changed 15 years ago by Jean-Paul Calderone

Cc: Jean-Paul Calderone added
Keywords: review removed
Owner: changed from Jean-Paul Calderone to Ralph Meijer
Status: assignednew

The added docstrings are excellent.

The code being refactored is mainly covered by existing tests, although some of it only indirectly (eg, there are no unit tests for ComponentInitiatingInitializer, though all of its lines are at least exercised indirectly by other tests). There doesn't seem to have been any existing test coverage for Service. However, since Service isn't new in this branch, I don't know that this lack needs to block the merge of this branch (if it doesn't, though, please create a ticket for adding test coverage).

I think IComponentService would be an improvement on IService. Likewise, if there is a default implementation, ComponentService makes more sense to me. This is generally keeping with the naming convention used through the rest of Twisted (ie, twisted.mail.imap4.IMAP4Server).

A convention which seems to be emerging is for interface modules to be named ipackage. For example, twisted.words.jabber.ijabber. This hasn't really solidifed yet, and it isn't used as widely through Twisted yet (though newer interface modules seem to be following it at least some of the time). Either ijabber or interfaces would be fine with me.

Given the amount of code in Service, perhaps it doesn't even need to exist. Given that it wasn't added in this branch though, it sounds like another change to be made in another branch. Removing it should put glyph's concern that there is only one implementation to rest, too.

So in summary, add unit tests for ComponentInitiatingInitializer, create some tickets for the other fixes, and then merge.

comment:9 Changed 15 years ago by Ralph Meijer

  • Regarding the location of Interface definitions, I created #2177.
  • Regarding the name and purpose of IService, I created #2178.
  • Added tests for ComponentInitiatingInitializer.

Merging.

comment:10 Changed 15 years ago by Ralph Meijer

Resolution: fixed
Status: newclosed

(In [18481]) Refactor component handshake as stream initializer.

Author: ralphm Reviewer: exarkun, glyph Fixes #1995

comment:11 Changed 11 years ago by <automation>

Owner: Ralph Meijer deleted
Note: See TracTickets for help on using tickets.