[sugar] review: gadget buddy search code

[Dafydd Harries]

Ar 15/05/2008 am 15:04, ysgrifennodd Guillaume Desmottes:
> Le mercredi 14 mai 2008 à 22:52 +0100, Dafydd Harries a écrit :
> > > +# FIXME: add more types
> > > +type_to_str = {str: 'str'}
> > > +str_to_type = {'str': str}
> > > +
> > 
> > We should check which types the buddy/activity interfaces support for
> > properties, and test each of those types.
> > 
> 
> Gabble supports the following types: str, bytes, int, uint and bool.
> 
> I started to write tests for parse_properties and properties_to_xml but
> I encountered a problem.
> Currently we store properties in a dict like {'key': (type, 'value')} as
> {'color': (str, 'red')} for example.
> 
> But how should I store an uint as Python doesn't have an uint type?
> Using dbus type? Or we could store the string representation of the type
> instead of a python type ('str' instead of str).
> 
> And how should we store the value? Using the type-converted value (so
> the numerical value for int/uint, True/False for bool, etc) or the
> string version?

The only thing the value representation matters for is searching. Storing them
as strings should be ok as long as there is only one string representation for
each value.

I'm not sure that the representation of the type matters. We should do
whatever is simplest. I suspect that might be storing the types as strings,
but I'm not sure.

I'm not sure about uints. Your suggestion of using the D-Bus type seems best.

> > > @@ -46,10 +50,25 @@ def parse_properties(elem):
> > >          if type is None or name is None or value is None:
> > >              continue
> > >
> > > -        properties[name] = (type, value)
> > > +        try:
> > > +            properties[name] = (str_to_type[type], value)
> > > +        except KeyError:
> > > +            continue
> > 
> > We shouldn't just ignore failed conversions. We should throw an error, and
> > make the caller return an error to the client. Perhaps the exception class
> > would have some convenience code for generating the error stanza.
> > 
> Good idea. Implemented.


> +            self.log("Ignore activity properties message from %s containing an invalid property"

This should be in the present tense ("Ignoring...") or the past tense
("Ignored..."). I think I prefer the former.

> > >  +
> > >  +        for elem in _query.children:
> > 
> > I'm not sure we should support answering multiple queries in one IQ. I'm not
> > sure if it does any harm, but I haven't seen other XMPP protocols allow it.
> > 
> 
> Don't know, I implemented the protocol described on
> http://wiki.laptop.org/go/XMPP_Component_Protocol

Heh, ok.

> > There's a lot of repeated/boilerplate code in these tests that makes them hard
> > to read. We should try and factor out the common elements, either by adding
> > some convenience functions (easier), or developing a framework that allows us
> > to write tests directly in terms of XMPP stanzas sent and received (harder,
> > but I think a bigger win). I imagine the second option involving having a
> > directory containing text files, where each file is a test consisting of a
> > trace of stanzas sent and received, similar to what you can do with Python
> > doctests. The format might be something like:
> > 
> >   -> <iq type="get" from="bob at foo.org" to="gadget at bar.org">
> >   ->   ...
> >   -> </iq>
> > 
> >   <- <iq type="result" from="gadget at bar.org" to="bob at foo.org">
> >   <-  ...
> >   <- </iq>
> > 
> > The main difficulty I see with this approach is that it's harder to set up
> > specific scenarios, but perhaps we can allow inserting Python code as well.
> > 
> >   >>> model.buddy_add(...)
> >   >>> model.activity_add(...)
> > 
> >   -> <iq type="get" from="bob at foo.org" to="gadget at bar.org">
> >   ...
> > 
> > Another option would be to just use doctests, with some convenience code for
> > sending/receiving stanzas.
> > 
> >   >>> send('''
> >   ... <iq type="get" from="bob at foo.org" to="gadget at bar.org">
> >   ... ...
> >   ... </iq>''')
> > 
> >   >>> print recv()
> >   <iq type="result" from="gadget at bar.org" to="bob at foo.org">
> >   ...
> >   </iq>
> > 
> 
> 
> I totally agree, having "XML oriented" test would be a big win. But I
> think we could do that as as separated branch so this test refactoring
> won't block the merge.

Sure. But:

+class BuddyIqTest5(TestCase):
+    """buddy multi query with two results"""

+class BuddyIqTest5(TestCase):
+    """buddy query with an invalid property"""

Let's rename the second one to BuddyIqInvalidPropertyTest or something like
that.

Otherwise, go ahead and merge.

-- 
Dafydd