Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Python 3 support #38

Merged
merged 1 commit into from
Jan 9, 2019
Merged

Python 3 support #38

merged 1 commit into from
Jan 9, 2019

Conversation

timonegk
Copy link
Contributor

Some small changes to make urdf_parser_py able to run in python3.

Copy link
Contributor

@clalancette clalancette left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All of the changes look OK to me (in that they probably won't break anything on python 2), with the possible exception of the addition of .decode(). I've had trouble with that before; have you tested on both python2 and python3 with that in place?

@timonegk
Copy link
Contributor Author

Even though .decode() would work, I think that providing the encoding='unicode' parameter is the better solution because it is easier to understand. In the background, both are the same (see the lxml source). I will edit my commit in a second.

@eacousineau
Copy link
Contributor

Should we perhaps revive Travis CI for Python3-only testing, if we want to vet PRs to ensure that they don't break anything? (at least until the buildfarm supports that configuration?)

@nzlz
Copy link

nzlz commented Dec 27, 2018

Hey guys, could you accept this PR? I faced the same issue and had to create a solution like the one posted here. Using the official repo is always more convenient.

I can create the PR to a new branch if this is the issue.

Copy link
Contributor

@clalancette clalancette left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me.

@sloretz unless you have objections here, I'll merge this later today.

@clalancette clalancette merged commit 29ca3b3 into ros:melodic-devel Jan 9, 2019
@timonegk timonegk deleted the python3 branch January 9, 2019 21:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants