ros-geographic-info / geographic_info

ROS packages for geographic information
http://ros.org/wiki/geographic_info
66 stars 62 forks source link

Python3 fixes #35

Closed klintan closed 4 years ago

klintan commented 4 years ago

Some minor py3 fixes to be able to use this for open_street_map package.

The most invasive one is this one:

for wid in range(self.n_points):
            self.way_point_ids[str(self.points[wid].id.uuid)] = wid

self.points[wid].id.uuid is a list, and a list can't be a key in a dict, so need to convert to string. Don't think it will ever affect anything in a negative way, but just wanted to highlight it.

Other changes should be straightforward.

edit: saw that there was an issue, to get this to work for both py2 and py3, and these changes would actually work in both I think (haven't tested py2 though)

SteveMacenski commented 4 years ago

Dumb question: isnt ROS2 python3?

Considering python2 is EOL and I believe ROS2 is all python3 (please correct me), I dont think it working with python2 is important.

klintan commented 4 years ago

Dumb question: isnt ROS2 python3?

Considering python3 is EOL and I believe ROS2 is all python3 (please correct me), I really couldn’t care if it works with python2.

You are def correct, so not really sure why I mentioned it, guess if they want to just grab these changes and apply them on master as well it could be done.

SteveMacenski commented 4 years ago

Does the test pass with those changes?

If so, we can merge

klintan commented 4 years ago

Does the test pass with those changes?

If so, we can merge

Rancolcon-test and colcon test-results which came up OK , but seems when running the python tests (pytest geodesy/tests) there are some issues. Will investigate and fix.

klintan commented 4 years ago

OK should be fixed now, UniqueID used to make things a bit easier with a bunch of helper methods, the test code now is a bit more verbose. I think the tests still tests the same thing though.

Also the setup.py eventually needs to be changed and a setup.cfg added to make it a "full" ROS2 package. Or perhaps it's done this way because it's only used as a library and not a node? (regardless distutils is no longer used so should probably be setuptools instead. :) )

SteveMacenski commented 4 years ago

Thanks for your diligence