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

Actually send the length byte in python 3 #759

Closed
wants to merge 4 commits into from

Conversation

eric-wieser
Copy link
Contributor

Fixes regresssion of #717

Calling `str` will mask bugs where a non-string object was passed in, and is not guaranteed to use utf8 encoding
dirk-thomas pushed a commit that referenced this pull request Mar 2, 2016
dirk-thomas pushed a commit that referenced this pull request Mar 2, 2016
@dirk-thomas
Copy link
Member

Thank you for the patch. The first commit is a crucial fix (0752719). Since the remaining three commits only refactor the code I squashed them into one (7a3d14d).

@dirk-thomas dirk-thomas closed this Mar 2, 2016
@eric-wieser
Copy link
Contributor Author

I'm slightly worried by how this failed the CI...

@dirk-thomas
Copy link
Member

I am not 😉 The Jade job passed and the Indigo job just had a single failing test which is sadly flaky.

@eric-wieser
Copy link
Contributor Author

I'm guessing there are no python 3 tests on travis right now. Does ROS even official support Python 3 before ROS 2.0?

@dirk-thomas
Copy link
Member

We try to support users which build ROS 1 from source. Commonly it is only the case on other platforms like Gentoo that they want to use Python 3. I don't expect that any ROS 1 distribution in the near future will switch to Python 3 simply because the community does not want to deal with the effort necessary to update existing code.

Btw. the pull request testing doesn't use Travis at all but the ROS Jenkins server from build.ros.org. And yes, it only tests with Python 2.

@dirk-thomas
Copy link
Member

FYI this patch needed fixing as proposed in #759.

@eric-wieser
Copy link
Contributor Author

Did you mean to link to the same page you posted on?

@dirk-thomas
Copy link
Member

Of course - not 😉 #789

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.

None yet

2 participants