Add XPath information when a parse error is encountered, rebased on indigo-devel

[eacousineau]

Please see original PR #8 for details.

@clalancette Sorry for the delay, but I've rebased this feature on the latest revision of indigo-devel.
Please let me know if there are any other changes that you would like to see.

Thanks!

[clalancette]

Hi @eacousineau, sorry for the delay. So, I looked over this code a bit. I like the final output of it, and the code itself seems pretty clean. However, I'm worried about changing the API of some of the methods to either take the path or other arguments in. In particular, since this is Python, I'm worried that downstream projects may be using some of those methods, and the change to the function signature will affect them. Indeed, if I check out srdfdom (https://github.com/ros-planning/srdfdom) and calibration (https://github.com/ros-perception/calibration) into my workspace, and use this branch, some of their tests start failing. I think we'll have to change some of this patch to maintain that backwards compatibility, particularly in indigo.

[eacousineau]

@clalancette Ahhh, that makes sense! I am not sure of any mechanisms, at least at the moment, to try and maintain the API, but will tinker around with it and see if there's a way to prevent breaking srdfdom and calibration. Thanks for catching that!

[eacousineau]

Whoops, looks like it was just an issue of me not updating the backwards-compatible xml_reflection.core.Object.parse. The calibration and srdfdom packages, at present, are not extending (Element|Attribute).read_xml.
I've fixed this on this branch, and have tested with these two packages, and the tests now pass.

$ source /opt/ros/indigo/setup.bash; mkcd build && cmake .. && source devel/setup.bash && make -j4 && make -j4 run_tests && make test
...
100% tests passed, 0 tests failed out of 21

I have also made changes to ensure the API is backwards-compatible, just in case.

Er... Realized that those changes are not at all backwards-compatible haha, will see if I can redo that. Stay tuned.

EDIT: I've reverted the backwards compatibility commit, as it does not actually do that. (This allows core methods to be called with only read_xml(node), but it does not permit existing extensions to be called as read_xml(node, path), as this will have extra arguments).

I have tried looking into the following options, with the attempts in this branch:
https://github.com/eacousineau/urdf_parser_py/commits/feature/xpath_indigo_backward_compat

1. Dynamically add a path attribute to each XML node. This does not work: The lxml.etree._Element object is a built-in type (from C++, I believe), and as such cannot be extended. I considered creating a wrapper for this Element object, but this would ultimately break any existing compatibility as well (if they use any intense functionality associated with this object, or if there are type checks to ensure that it is an Element type).
2. Create a simple map, associating a node instance with its path in the current parsing. This does not work with weakref.WeakKeyDictionary, since the objects are built-ins, so for the time being, I have implemented this as a simple dict, which will not be thread-safe for multiple objects parsing.
3. Create a full new wrapper class and either:

• (a) contain the XML node and path explicitly (this is what I desire, but definitely would break compatibility), or
• (b) mirror the original XML methods and allow adding new properties. This seemed like quite a bit of work and seemed like it may break existing connections anyway.

Overall, if a change were to be made, my favor is with 3(a) [not even backwards compatible :P] as all of the other ones are dirty as all get out.
What do you think?

[clalancette]

So, let me ask this question; how badly do you want this in Indigo, Jade, and/or Kinetic? I have no problem changing this API for Lunar going forward, but for the older, long-term distributions, I'm reluctant to change things. In particular, we don't know about any out-of-tree code that is going to depend on this, and I hate to break things that are already working. What do you think of merging this for just Lunar?

[eacousineau]

This is purely aesthetic (and not even "critical" aesthetics), so I could definitely go for merging it into Lunar. I can make that 3(a) change, and then resubmit rebased onto Lunar. Does that sound good to you?

[clalancette]

Actually, do we even need to make the 3(a) change? If we are just declaring backwards-incompatibility, can we go with the code as-is? Or am I missing something?

[clalancette]

I rebased this onto the latest (I caused all of the merge conflicts, so it was my responsibility to do that). I think I'm OK with putting this into a melodic-devel branch, given the backwards-compatibility concerns. @sloretz, do you have any issue with that?

[sloretz]

@clalancette melodic release for this sounds good to me.

[clalancette]

I've now run a prerelease with this PR against as many packages as possible. There were no failures relevant to this PR that I saw. I also did a by-hand inspection of the python code of the direct downstream users, just to see if there were any obvious breakage. This inspection is incomplete by the nature of Python, but at a basic glance, nothing looks like it will break. I've also retargeted this to melodic-devel. @sloretz unless you have any last-minute objections, I'm going to merge this in soon.