Dan Kenigsberg has posted comments on this change. Change subject: Improve dom xml gereration ......................................................................
Patch Set 4: I would prefer that you didn't submit this (5 inline comments) much like Vinzenz, I am a bit weary of this change before we have comprehensive libvirtvmTest. We achingly need a test that receives a conf, produces a domxml, and compares it with a pre-calculated one. But I suppose that we can still take this patch if it touches only unittested functions and undergone enough verification. .................................................... File vdsm/libvirtvm.py Line 550: Line 551: Line 552: class XMLElem(object): Line 553: Line 554: def __init__(self, tagName, text=None, attrs={}): how about defining this with **attr? it would make usage slightly cooler. Line 555: self.doc = xml.dom.minidom.Document() Line 556: self.elem = self.doc.createElement(tagName) Line 557: self.setAttrs(attrs) Line 558: if text is not None: Line 551: Line 552: class XMLElem(object): Line 553: Line 554: def __init__(self, tagName, text=None, attrs={}): Line 555: self.doc = xml.dom.minidom.Document() I have no strong opinion about Zhou's comment (receiving doc as an arg instead of creating one for every XMLElem), but this seems a bit wasteful of resources. If you keep it, I don't think you need it as a data member of this object - it is never used out of __init__. Line 556: self.elem = self.doc.createElement(tagName) Line 557: self.setAttrs(attrs) Line 558: if text is not None: Line 559: self.addTextNode(text) Line 552: class XMLElem(object): Line 553: Line 554: def __init__(self, tagName, text=None, attrs={}): Line 555: self.doc = xml.dom.minidom.Document() Line 556: self.elem = self.doc.createElement(tagName) is self.elem ever used directly? I feels like it should remain _private. Line 557: self.setAttrs(attrs) Line 558: if text is not None: Line 559: self.addTextNode(text) Line 560: Line 564: def setAttrs(self, attrs): Line 565: for attrName, attrValue in attrs.iteritems(): Line 566: self.elem.setAttribute(attrName, attrValue) Line 567: Line 568: def addTextNode(self, text): minidom uses the "append" verb. it may seem clumsier than "add", but let us conform. Line 569: textNode = self.doc.createTextNode(text) Line 570: self.elem.appendChild(textNode) Line 571: Line 572: def addChild(self, childName, text=None, attrs={}): Line 568: def addTextNode(self, text): Line 569: textNode = self.doc.createTextNode(text) Line 570: self.elem.appendChild(textNode) Line 571: Line 572: def addChild(self, childName, text=None, attrs={}): here too: appendChild seems a better name, and **attrs could be cooler. Line 573: child = XMLElem(childName, text, attrs) Line 574: self.elem.appendChild(child) Line 575: return child Line 576: -- To view, visit http://gerrit.ovirt.org/10054 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ib936740fcbeeee551e13abfed7fd91f2a159e351 Gerrit-PatchSet: 4 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Mark Wu <wu...@linux.vnet.ibm.com> Gerrit-Reviewer: Dan Kenigsberg <dan...@redhat.com> Gerrit-Reviewer: Mark Wu <wu...@linux.vnet.ibm.com> Gerrit-Reviewer: ShaoHe Feng <shao...@linux.vnet.ibm.com> Gerrit-Reviewer: Vinzenz Feenstra <vfeen...@redhat.com> Gerrit-Reviewer: Zhou Zheng Sheng <zhshz...@linux.vnet.ibm.com> Gerrit-Reviewer: oVirt Jenkins CI Server _______________________________________________ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches