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

fix(python): avoid removal of null characters in bindings #66

Merged
merged 4 commits into from
Sep 20, 2023

Conversation

domire8
Copy link
Member

@domire8 domire8 commented Sep 20, 2023

Description

The receive_bytes in python is indeed causing some troubles. The removal of the null characters at the end of the received buffer is not a good idea, because this will "destroy" clproto messages that have zeros (for example a zero joint state). Returning the buffer just like that is also not a good idea, because then clproto messages would be decoded, which will throw exceptions.

Unfortunately, I don't see a better way of handling this right now. I don't think we can make a general assumption about the messages that we receive, so its ultimately the users responsibility to correctly decode and rstrip the received message.

Review guidelines

Estimated Time of Review: 5 minutes

Checklist before merging:

  • Confirm that the relevant changelog(s) are up-to-date in case of any user-facing changes

@domire8
Copy link
Member Author

domire8 commented Sep 20, 2023

Also, is it okay to change the version in this PR such that I can tag it directly afterwards?

Copy link
Member

@eeberhard eeberhard left a comment

Choose a reason for hiding this comment

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

Classic message termination problems. I guess we could have anticipated it but oh well.

The tests are good but as a nitpick I would suggest a small usage example / python snippet in the README to illustrate how to handle bytes and strings when sending and receiving. Feel free to make an issue and do it another time (or a backlog item for @JasperTan97)

Also, is it okay to change the version in this PR such that I can tag it directly afterwards?

Yeah for sure, the release strategy now allows that. Separate release PRs are only needed if we decide to tag a release or write some change notes after the fact.

@domire8 domire8 merged commit e32f3b2 into main Sep 20, 2023
3 checks passed
@domire8 domire8 deleted the fix/receive-bytes branch September 20, 2023 19:24
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.

2 participants