Milan Zamazal has posted comments on this change. Change subject: virt: Make DomainDescriptor use XML helpers ......................................................................
Patch Set 8: (7 comments) Thank you all for the comments and suggestions, I hope things get clarified and we can make a progress. If we can agree on using default value argument in *find* then the major concept issues are probably resolved. Let's also not forget we can either discard the API or continue using it after the final switch to etree. I prefer making a clean API, because in case we decide to keep it or we stop before the final switch to etree for some serious reason, we can use the API without worries. But even then the API needn't be engraved into a stone at this moment, I think making progress beyond this patch is more important. https://gerrit.ovirt.org/#/c/55769/8/vdsm/virt/vmxml.py File vdsm/virt/vmxml.py: Line 78: result = [element] + result Line 79: return result Line 80: Line 81: Line 82: def dom_find_tag(element, tag, index=0): > I'm not convinced by the argument about common patterns in client code. I also don't like having both find_xxx and require_xxx, although for slightly different reasons: 1. It's unnecessary to double the number of helpers when we can simply use an argument; 2. it's easy to miss that find_xxx may return None. I like the suggestion about having a default argument. It solves all the problems (and obsoletes the need for `required' argument). Line 83: """ Line 84: Find a DOM element specified by given arguments. Line 85: Line 86: :param element: DOM object to be searched for given `tag` Line 98: except IndexError: Line 99: return None Line 100: Line 101: Line 102: def dom_find_attr(element, tag, attribute=True, index=0): > Why have an api accepting element when we want to find the attribute of a s Because dom_find_attr(element, 'foo', 'bar') is easier to use than dom_attr(dom_find_tag(element, 'foo'), 'bar') or tag_element = dom_find_tag(element, 'foo') dom_attr(tag_element, 'bar') or even tag_element = dom_find_tag(element, 'foo', default=None) if tag_element is not None: dom_attr(tag_element, 'bar') (consider something like if dom_find_attr(element, 'foo', 'bar', default=None) == 'baz': ... ) Line 103: """ Line 104: Find attribute values of a DOM element specified by given arguments. Line 105: Line 106: :param element: DOM object to be searched for given `tag` Line 125: if subelement is None: Line 126: if isinstance(attribute, basestring): Line 127: return '' Line 128: else: Line 129: return {} > Why we should care about argument type? Why should be return different retu It's too complicated, I should use separate methods here instead of switching on the argument type. Line 130: return dom_attr(subelement, attribute) Line 131: Line 132: Line 133: def dom_tag(element): Line 139: :returns: tag of the element Line 140: :rtype: basestring Line 141: Line 142: """ Line 143: return element.tagName > Why do we need this? what is the etree way that this should hide? We need it for the minidom->etree transition period (at least), because it's different in minidom and etree. Line 144: Line 145: Line 146: def dom_attr(element, attribute): Line 147: """ Line 151: :type element: DOM object Line 152: :param attribute: attribute name to look for, or a sequence of attribute Line 153: names to look for, or True to return all attribute values of the given Line 154: element Line 155: :type attribute: basestring, or sequence of basestrings, or True > Please accept only one type for attribute, this does not make any sense. Ac I agree, it should be split into two helpers: 1. for retrieving just a simple attribute; 2. for retrieving multiple attributes, perhaps in the way you suggest in another comment. Line 156: :returns: the corresponding attribute values or empty string (if Line 157: `attribute` is basestring and no corresponding attribute is found) Line 158: :rtype: basestring if `attribute` is string; dictionary of Line 159: attribute names as keys and their corresponding values otherwise Line 180: :rtype: basestring or NoneType Line 181: Line 182: """ Line 183: if element is None: Line 184: return None > This is very bad idea, result of having functions returning None. The calle OK, I think it's used in only two places now so it doesn't matter much and we can add the extra condition to that places. Line 185: child_node = element.firstChild Line 186: if child_node is None: Line 187: return None Line 188: return child_node.nodeValue Line 184: return None Line 185: child_node = element.firstChild Line 186: if child_node is None: Line 187: return None Line 188: return child_node.nodeValue > Why not return always a string? If there is not child node, return empty st Yes, let's always return string from this method. Line 189: Line 190: Line 191: def has_channel(domXML, name): Line 192: domObj = etree.fromstring(domXML) -- To view, visit https://gerrit.ovirt.org/55769 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ib169735936d19171ff8b8d127666d7258c308f34 Gerrit-PatchSet: 8 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Milan Zamazal <mzama...@redhat.com> Gerrit-Reviewer: Francesco Romani <from...@redhat.com> Gerrit-Reviewer: Martin Polednik <mpoled...@redhat.com> Gerrit-Reviewer: Milan Zamazal <mzama...@redhat.com> Gerrit-Reviewer: Nir Soffer <nsof...@redhat.com> Gerrit-Reviewer: gerrit-hooks <automat...@ovirt.org> Gerrit-HasComments: Yes _______________________________________________ vdsm-patches mailing list -- vdsm-patches@lists.fedorahosted.org To unsubscribe send an email to vdsm-patches-le...@lists.fedorahosted.org
