[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