Nir Soffer has posted comments on this change. Change subject: virt: Make DomainDescriptor use XML helpers ......................................................................
Patch Set 8: (9 comments) 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): > TL;DR: I'm not convinced by the argument about common patterns in client code. First, api should be created for any code, not the code we currently have, and second, having lot of code using bad return None pattern Is even worse. Finally, the module api should be consistent, we don't want to have both find_xxx and require_xxx for every helper in this module or some of the helpers. We should use consistent pattern for all helpers. They can all return None when there is no result, or raise NotFound. A possible way to get both behaviours in a pythonic way is to have api like getattr(obj, name, default). If you want to ignore missing result, you use None or False in the default. 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): > Same here Why have an api accepting element when we want to find the attribute of a sub element? The caller should get the sub element and get the attribute of the sub element. 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 return value, when both of them are "empty"? 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? 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. Accept either string or list or boolean. Looks like you are trying to mix several helpers into one. 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 159: attribute names as keys and their corresponding values otherwise Line 160: Line 161: """ Line 162: if attribute is True: Line 163: result = {a: dom_attr(element, a) for a in element.attributes.keys()} The user can call us with element.attributes.keys(), or we can just return all attributes if the user did not specify anything. Line 164: elif isinstance(attribute, (list, tuple,)): Line 165: result = {a: dom_attr(element, a) for a in attribute} Line 166: else: Line 167: # Minidom returns unicodes, except for empty strings. Line 164: elif isinstance(attribute, (list, tuple,)): Line 165: result = {a: dom_attr(element, a) for a in attribute} Line 166: else: Line 167: # Minidom returns unicodes, except for empty strings. Line 168: result = element.getAttribute(attribute) Accepting *attributes will make this helpr much more consistent and easy to use: def dom_attrs(element, *attributes): if not attributes: attributes = element.attributes.keys() return {a: dom_attr(element, a) for a in attributes} This always return a dict, with some of the attributes and values you ask. Line 169: return result Line 170: Line 171: Line 172: def dom_text(element): 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 caller should check the value before calling. 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 string. If we have a child, return its value. 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 https://lists.fedorahosted.org/admin/lists/vdsm-patches@lists.fedorahosted.org
