Dan Kenigsberg has posted comments on this change. Change subject: caps: replacing minidom with ElementTree in caps.py ......................................................................
Patch Set 9: (2 comments) https://gerrit.ovirt.org/#/c/41078/9/vdsm/caps.py File vdsm/caps.py: Line 207: if capabilities is None: Line 208: capabilities = _getFreshCapsXMLStr() Line 209: Line 210: caps = ET.fromstring(capabilities) Line 211: host = caps.iter(tag='host').next() > the problem here is that I don't really like this iterator-based API. I _re When we need only one element (like here) I can see why host = caps.find('host') is nicer. but when we want to iterate over all elements of certain tag, I don't see the benefit of findall(). To the contrary - creating an intermediate list is redundant. Line 212: cells = host.iter(tag='cells').next() Line 213: Line 214: sockets = set() Line 215: siblings = set() Line 242: None if libvirt does not report the live Line 243: snapshot support (as in version <= 1.2.2), Line 244: ''' Line 245: features = guest.iter(tag='features') Line 246: if not features: iter() never returns a "false" object. It may return an empty iterator, but you cannot check for it like this. Here, find() is *clearly* better fitting, because you can check if it returned None. Line 247: return None Line 248: Line 249: for feature in features.next().iter(tag='disksnapshot'): Line 250: value = feature.get('default') -- To view, visit https://gerrit.ovirt.org/41078 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I8c1ca58807515922347e2255c77b4f950decd619 Gerrit-PatchSet: 9 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Marcin Mirecki <[email protected]> Gerrit-Reviewer: Dan Kenigsberg <[email protected]> Gerrit-Reviewer: Francesco Romani <[email protected]> Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Marcin Mirecki <[email protected]> Gerrit-Reviewer: Vinzenz Feenstra <[email protected]> Gerrit-Reviewer: [email protected] Gerrit-HasComments: Yes _______________________________________________ vdsm-patches mailing list [email protected] https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
