Milan Zamazal has posted comments on this change.

Change subject: virt: Make DomainDescriptor use XML helpers
......................................................................


Patch Set 9:

(10 comments)

I tried to address all the suggestions for improvements, so hopefully we can 
move forward now with the conversion.

https://gerrit.ovirt.org/#/c/55769/8/vdsm/virt/vmxml.py
File vdsm/virt/vmxml.py:

Line 46: def parse_xml(xml_string):
Line 47:     """
Line 48:     Parse given XML string to DOM element and return the element.
Line 49: 
Line 50:     :param xml_string: XML string to parse
> This looks like element_to_xml
I renamed both the parse/export methods to avoid these concerns.
Line 51:     :type xml_string: str
Line 52:     :returns: DOM element created by parsing `xml_string`
Line 53:     :rtype: DOM element
Line 54: 


Line 50:     :param xml_string: XML string to parse
Line 51:     :type xml_string: str
Line 52:     :returns: DOM element created by parsing `xml_string`
Line 53:     :rtype: DOM element
Line 54: 
> What is DOM object?
I'm intentionally vague here since it differs in minidom/etree and nobody 
should depend on the particular type during the transition. We can change that 
after the transition is completed.

I changed the terminology from "DOM object" to "DOM element". Not sure whether 
it's better.
Line 55:     """
Line 56:     return xml.dom.minidom.parseString(xml_string)
Line 57: 
Line 58: 


Line 59: def export_xml(element):
Line 60:     """
Line 61:     Export given DOM element to XML string.
Line 62: 
Line 63:     :param element: DOM element to export
> How about find_all()?
Done
Line 64:     :type element: DOM element
Line 65:     :returns: XML corresponding to `element` content
Line 66:     :rtype: basestring
Line 67: 


Line 73:     """
Line 74:     Return an iterator over all DOM elements with given `tag`.
Line 75: 
Line 76:     `element` may is included in the result if it is of `tag`.
Line 77: 
> Please avoid this () hack and format the code as pep8 wants.
Done
Line 78:     :param element: DOM element to be searched for given `tag`
Line 79:     :type element: DOM element
Line 80:     :param tag: tag to look for
Line 81:     :type tag: basestring


Line 74:     Return an iterator over all DOM elements with given `tag`.
Line 75: 
Line 76:     `element` may is included in the result if it is of `tag`.
Line 77: 
Line 78:     :param element: DOM element to be searched for given `tag`
> Why results.insert(0, element)?
Yes, generator is better, let's use it.
Line 79:     :type element: DOM element
Line 80:     :param tag: tag to look for
Line 81:     :type tag: basestring
Line 82:     :returns: all elements with given `tag`


Line 78:     :param element: DOM element to be searched for given `tag`
Line 79:     :type element: DOM element
Line 80:     :param tag: tag to look for
Line 81:     :type tag: basestring
Line 82:     :returns: all elements with given `tag`
> I also don't like having both find_xxx and require_xxx, although for slight
Renamed to find_first, `index' argument removed, `default' argument added.
Line 83:     :rtype: sequence of DOM elements
Line 84: 
Line 85:     """
Line 86:     if isinstance(element, xml.dom.minidom.Element) and \


Line 98:     :type element: DOM element
Line 99:     :param tag: tag to look for
Line 100:     :type tag: basestring
Line 101:     :param default: any object to return when no element with `tag` 
is found;
Line 102:       if not given then `NotFound` is raised in such a case
> Because
`attribute' is now just the attribute name, `index' argument removed.
Line 103:     :returns: the matching element or default
Line 104:     :raises: NotFound -- when no element with `tag` is found and 
`default` is
Line 105:       not given
Line 106: 


Line 151: 
Line 152: def attr(element, attribute):
Line 153:     """
Line 154:     Return attribute values of `element`.
Line 155: 
> I agree, it should be split into two helpers: 1. for retrieving just a simp
Just a simple `attribute' value is accepted now.
Line 156:     :param element: the element to look the attributes in
Line 157:     :type element: DOM element
Line 158:     :param attribute: attribute name to look for
Line 159:     :type attribute: basestring


Line 180:     child_node = element.firstChild
Line 181:     if child_node is None:
Line 182:         return ''
Line 183:     return child_node.nodeValue
Line 184: 
> OK, I think it's used in only two places now so it doesn't matter much and 
Done
Line 185: 
Line 186: def has_channel(domXML, name):
Line 187:     domObj = etree.fromstring(domXML)
Line 188:     devices = domObj.findall('devices')


Line 184: 
Line 185: 
Line 186: def has_channel(domXML, name):
Line 187:     domObj = etree.fromstring(domXML)
Line 188:     devices = domObj.findall('devices')
> Yes, let's always return string from this method.
Done
Line 189: 
Line 190:     if len(devices) == 1:
Line 191:         for chan in devices[0].findall('channel'):
Line 192:             targets = chan.findall('target')


-- 
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: 9
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Milan Zamazal <mzama...@redhat.com>
Gerrit-Reviewer: Francesco Romani <from...@redhat.com>
Gerrit-Reviewer: Jenkins CI
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

Reply via email to