[Code-review] review: Gadget act-props branch
Dafydd Harries
dafydd.harries at collabora.co.uk
Mon Aug 11 15:28:17 EDT 2008
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.
> > >+ 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'
If None means we don't check the uri, then a <property> element with a
different namespace won't get ignored.
> > 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.
--
Dafydd
More information about the Code-review
mailing list