[Code-review] review: Gadget members branch
guillaume.desmottes at collabora.co.uk
Tue Aug 12 11:22:04 EDT 2008
Le lundi 11 août 2008 à 14:57 +0100, Dafydd Harries a écrit :
> >+ activity_elem = message.addElement((ns.ACTIVITY, 'activity'))
> >+ activity_elem['id'] = str(activity.id)
> >+ activity_elem['room'] = activity.room
> >+ activity_elem['view'] = self.id
> This seems like something we should factor out. Then we can just write:
> Perhaps we need two variants, depending on whether we want to include the properties or not.
> message.addChild(activity_to_xml(activity, False))
Actually I didn't have an activity_to_xml method in the component class.
I adapted it to use it in all cases.
> Maybe it should be a method on the activity class.
I do like to have the model completely XMPP ignorant.
> >+ def buddy_properties_to_xml(self, buddy):
> >+ if not self.model.has_buddy(buddy):
> >+ log.msg("unknown member %s; can't add their properties"
> >+ "to query reply" % buddy)
> >+ return None
> >+ buddy = self.model.buddy_by_id(buddy)
> >+ properties = domish.Element((ns.BUDDY_PROP, 'properties'))
> >+ for node in properties_to_xml(buddy.properties):
> >+ properties.addChild(node)
> >+ # FIXME: We should send buddy properties only if the user
> >+ # doesn't know this JID yet (he doesn't have any open view
> >+ # containing it).
> >+ return properties
> This refactoring looks a bit strange: it mixes together the job of checking
> that the buddy ID is valid with converting the data to XML. I think it would
> be better for this code to take a buddy object, and for the calling function
> to be responsible for ensuring it's dealing with a valid ID. Also, the comment
> doesn't really make sense in this context.
You're right. I dropped this function and used properties_to_xml
> Why might we have an invalid buddy ID to begin with?
If a contact joins the activity room but is not subscribed to Gadget.
Then Gadget doesn't know his properties.
> >+ log.msg("can't update
> activity members: full jid of %s is missing"
> >+ % nick)
> This message is a bit misleading: better to say "real" instead of "full".
> Also, "JID" should be capitalised.
> >+ if type is None:
> >+ log.msg('room %s has member %s' % (room_jid, nick))
> >+ activity.members.add(real_jid)
> This check should be an early return at the beginning of the function. It
> would be very strange if the type was not None while we were joining the room.
> We should probably do:
> type = presence.getAttribute('type', 'available')
> And then compare against that. But that's an aesthetic change.
> >+ self.server.addOnetimeObserver('//message', self.message3)
> I wonder if we could automate this.
> class StanzaSeriesTest(unittest.TestCase):
> def __init__(self):
> # Chain stanza callbacks together.
> callbacks = [getattr(self, name)
> for name in dir(self)
> if name.startswith('stanza')]
> def wrap(callback, next):
> def wrapped(elem):
> self.addOnetimeObserver('//*', next)
> return wrapped
> for callback, next in zip(callbacks[:-1], callbacks[1:]):
> self.__dict__[f.__name__] = wrap(f)
> This is untested, but you see the principle. One disadvantage is that callback
> names might be less self-descriptive.
I'm not convince about this. Current code make things clearer and the
test workflow is easier to understand (which is more important than
code's size for tests IMHO).
More information about the Code-review