Remove lxml dependency #57
Conversation
Co-authored-by: Tully Foote <tfoote@osrfoundation.org>
Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>
Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>
Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>
Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>
Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>
Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>
Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>
| # TODO(eacousineau): This does not preserve attribute order. Fix it. | ||
| dom = minidom.parseString(ET.tostring(rootXml)) | ||
| xml_string = "" | ||
| lines = dom.toprettyxml(indent=" ").split("\n") |
There was a problem hiding this comment.
I had two major comments on the original PR.
The first one was about pretty printing the output. That one isn't super-important, but was something that the old one did. It looks like this one is doing that as well, which is good, and takes care of that point.
The second point was about preserving XML comments, which lxml does, but minidom does not. I guess we have to answer whether that is an important use case, and whether we can make that happen with minidom. I don't know that answer to either one.
There was a problem hiding this comment.
It looks like the melodic-devel branch is explicitly filtering XML comments out. Is there another API in this repo that gives access to them?
urdf_parser_py/src/urdf_parser_py/xml_reflection/basics.py
Lines 34 to 39 in aa7199f
There was a problem hiding this comment.
I was originally worried about round-tripping through this API, like:
from urdf_parser_py.urdf import URDF
with open('robot.urdf', 'r') as infp:
robot = URDF.from_xml_string(infp.read())
print(robot.to_xml_string())
But you are right, we are already stripping out those comments. Personally I think that is a bug, but that is orthogonal to this PR. So I think we can proceed.
| @@ -580,7 +580,7 @@ def to_xml(self): | |||
| """ Creates an overarching tag and adds its contents to the node """ | |||
| tag = self.XML_REFL.tag | |||
| assert tag is not None, "Must define 'tag' in reflection to use this function" # noqa | |||
| doc = etree.Element(tag) | |||
| doc = ET.Element(tag) | |||
There was a problem hiding this comment.
I guess we are inheriting ET by importing from basics.* at the top, but I think it would be a bit better to explicitly do it at the top of this file:
from xml.etree import ElementTree as ET
Minor comment, though, I'll leave it to you whether you think we should do that.
There was a problem hiding this comment.
I removed the * import from core.py in 0ae7259 . sdf and urdf also import *, but I'd like to fix that in a follow up PR.
Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>
There was a problem hiding this comment.
I took a quick look at 0ae7259 , just to try to make sure we got all of the imports. Looks pretty good to me, though we'd need more comprehensive tests to be sure.
Good to merge from me, assuming it passes the tests we already have.
Continues #40 which is a continuation from #32. This removes the dependency on
lxml.