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

Python3 compat #47

Merged
merged 1 commit into from Sep 12, 2019
Merged

Python3 compat #47

merged 1 commit into from Sep 12, 2019

Conversation

jbredeche
Copy link
Member

No description provided.

@@ -285,7 +286,7 @@ def _send_to_socket(sck, msg):
send_to_tracer method.
"""
sck.sendall(pack('>i', len(msg)))
sck.sendall(msg)
sck.sendall(str_to_bytes(msg, encoding='utf-8'))

Choose a reason for hiding this comment

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

I think we need to convert to bytes before computing length above. This would fail if we wrote any non-unicode data.

Copy link
Member Author

Choose a reason for hiding this comment

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

good catch, thanks.

Choose a reason for hiding this comment

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

Though, this is also a case where bytes really feels like the right interface to me. "put something into socket" is definitely an operation that only makes sense for bytes. What do you think about lifting the bytes conversion into fmt_msg and fmt_error_msg, since those are already concerned with serialization of the messages we're going to put on the wire?

Copy link
Member Author

Choose a reason for hiding this comment

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

Makes sense, but it's a bigger change than I want to make right now. It looks like fmt_msg is used elsewhere (like with bound_cmd_manager in tracer.py) and can take an optional serial parameter. I'd want to spend time digging through all that.

Choose a reason for hiding this comment

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

Ah, yeah, I didn't realized that was used elsewhere. I guess this is fine for now. Can we document explicitly that this takes str (and thus that it expects unicode in py3)?

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