[Code-review] review: Gadget act-props branch

[Guillaume Desmottes]

Le lundi 11 août 2008 à 20:28 +0100, Dafydd Harries a écrit :
> Ar 11/08/2008 am 20:21, ysgrifennodd Guillaume Desmottes:
> > Le lundi 11 août 2008 à 14:07 +0100, Dafydd Harries a écrit :
> > > act-props:
> > > 
> > > >+        properties = activity_elem.addElement((ns.ACTIVITY_PROP, 'properties'))
> > > >+        for node in properties_to_xml(activity.properties):
> > > >+            properties.addChild(node)
> > > 
> > > We seem to do this a lot. Let's change it so properties_to_xml returns the
> > > <properties> element, then we can just say:
> > > 
> > >   activity_elem.addChild(properties_to_xml(activity.properties))
> > 
> > 
> > The old code used to do that and you asked me to change to this current
> > form :)
> 
> Hmm, I think it was slightly different. It used to be that you passed in the
> parent node, and properties_to_xml called addElement on it. I think when I
> asked you to change it, I didn't realise you could pass in existing elements
> to addChild.

Oh, I see. Good idea, I did that.

> > > >    for child in node.elements():
> > > >+        if child.uri == ns.ACTIVITY_PROP and child.name == 'properties':
> > > >+            for child in child.elements():
> > > >+                assert child.name == 'property'
> > > 
> > > I think it's better to just ignore unexpected elements:
> > > 
> > >   for child in child.elements():
> > >       if child.name != 'property':
> > >           continue
> > > 
> > > We could have a convenience function:
> > > 
> > >   def elem_find_children(elem, uri, name):
> > >       for child in elem.elements():
> > >           if (child.uri, child.name) == (uri, name):
> > >               yield child
> > > 
> > >       ...
> > > 
> > > So we could then write:
> > > 
> > >   for child in elem_find_chidlren(ns.ACTIVITY_PROP, 'property'):
> > >       ...
> > > 
> > > This also handles namespaces more correctly.
> > > 
> > 
> > Nice idea. I added this function to util.py but modified it a bit to
> > handle None as uri (as in this example <property> inherit its NS but
> > child.uri doesn't contain it).
> 
> Hmm, that doesn't seem right to me: the child should have the same .uri value.
> 
> >>> e = domish.Element(('http://foo', 'foo'))
> >>> child = e.addElement('bar')
> >>> e.toXml()
> u"<foo xmlns='http://foo'><bar/></foo>"
> >>> child.uri
> 'http://foo'
> 

humm, right, actually that's a bug in elem() which doesn't properly
manage namespace. I pushed (on dev.laptop.org) in a "elem-ns-bug" a test
exhibiting the problem. Would be good if you could take a look.

> If None means we don't check the uri, then a <property> element with a
> different namespace won't get ignored.

I agree, we should probably always check the uri, but we need to fix the
elem ns bug first.

> > > Otherwise, looks fine.
> > 
> > 
> > I pushed my changes, let me know if you're ok with them.
> 
> Does elem_find_children() have a test? If not, please add one.


I'm adding one.


	G