Skip to content

Fix serialization / deserialization of char array. #21

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

Merged
merged 2 commits into from
May 31, 2018
Merged

Fix serialization / deserialization of char array. #21

merged 2 commits into from
May 31, 2018

Conversation

moriyoshi
Copy link
Contributor

@moriyoshi moriyoshi commented May 31, 2018

This patch fixes the following issues:

  • Deserialization of char[] which contains bytes in range of 128 to 255.
    For example using unpack spec '>hh" on "\xff\xff" results in (-1, -1) and causes ValueError in bytes().
  • Correct handleing of surrogate pairs.
    Just fixing the above doesn't solve all the problems. Applying.decode('utf16-be') on a resulting buffer of such two bytes that represent surrogate pairs ends up in UnicodeDecodeError.
  • Debug logging of non-ASCII characters in range of U+1000 to U+FFFF
    I don't really think that it is a good idea to manipulate strings for debugging regardless of its being enabled or not.
  • Proper Python 2 support.
    In py2 bytes is merely an alias of str, which doesn't construct a sequence of bytes from a iterator.
  • Serialization of char[].
    It's not been implemented at all.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.7%) to 84.357% when pulling c26690f on moriyoshi:moriyoshi/fix-type-char-unpack into 8472ed9 on tcalmant:master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.7%) to 84.357% when pulling c26690f on moriyoshi:moriyoshi/fix-type-char-unpack into 8472ed9 on tcalmant:master.

@coveralls
Copy link

coveralls commented May 31, 2018

Coverage Status

Coverage decreased (-0.7%) to 84.317% when pulling 38c7b92 on moriyoshi:moriyoshi/fix-type-char-unpack into 8472ed9 on tcalmant:master.

@tcalmant tcalmant merged commit 2d48977 into tcalmant:master May 31, 2018
@tcalmant
Copy link
Owner

Thanks for your contribution!
I'll make a release after some tests and completing the AUTHORS file.

@moriyoshi
Copy link
Contributor Author

Thank you for looking into this!

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.

3 participants