[Code-review] review: Gadget members branch

Dafydd Harries dafydd.harries at collabora.co.uk
Wed Aug 13 10:52:00 EDT 2008


Ar 12/08/2008 am 16:22, ysgrifennodd Guillaume Desmottes:
> 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:
> > 
> >   message.addChild(activity_to_xml(activity))
> > 
> > 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.
> > 
> >   message.addChild(activity.to_xml(False))
> 
> I do like to have the model completely XMPP ignorant.

Good point!

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

Ah, right. Perhaps we should have a comment in the code to explain.

> > >+        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):
> >                   callback(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).

Hmm, perhaps the value is limited.

Ok, let's merge it.

-- 
Dafydd


More information about the Code-review mailing list