Skip to content

Fix msgpack issues and warnings #98

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

Conversation

alimanfoo
Copy link
Member

@alimanfoo alimanfoo commented Nov 6, 2018

This PR addresses a couple of issues relating to the new msgpack codec (codec ID "msgpack2") being introduced in numcodecs version 0.6.0:

  • The encoding argument to packb and unpackb is being deprecated and is generating a PendingDeprecationWarning. The arguments have been changed as recommended to use raw=False instead, with raw=True being available as an option. Two other relevant options, use_bin_type and use_single_float have also been surfaced from msgpack. Resolves msgpack: PendingDeprecationWarning: encoding is deprecated, Use raw=False instead #85.
  • Round-trip encoding/decoding of an array of Python bytes objects was broken in the legacy msgpack codec, and works correctly in the new msgpack2 codec with new arguments set to defaults. Some tests have been added to verify the behaviour of old and new codecs with respect to both bytes and str (unicode) objects. This resolves an issue in zarr (MsgPack codec is broken for array of bytes objects zarr-python#324) where the tests currently expect broken behaviour from msgpack for an array of bytes objects.

N.B., because default values for some arguments being passed through to msgpack in the newer codec have changed in this PR, the fixture for the "msgpack2" codec has been regenerated. This is OK because the codec has not actually been released yet.

For reference, the initial work to add the new msgpack2 codec was done in PR #77.

TODO:

  • Unit tests and/or doctests in docstrings
  • tox -e py37 passes locally
  • tox -e py27 passes locally
  • Docstrings and API docs for any new/modified user-facing classes and functions
  • Changes documented in docs/release.rst
  • tox -e docs passes locally
  • AppVeyor and Travis CI passes
  • Test coverage to 100% (Coveralls passes)

@alimanfoo alimanfoo force-pushed the msgpack-alimanfoo-20181106 branch from f447389 to aca7d6c Compare November 6, 2018 23:10
@alimanfoo alimanfoo added this to the 0.6.0 milestone Nov 6, 2018
@alimanfoo alimanfoo requested a review from jakirkham November 6, 2018 23:43
@alimanfoo
Copy link
Member Author

Any objections @zarr-developers/core-devs?

Copy link
Member

@jakirkham jakirkham left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks @alimanfoo :)

@alimanfoo alimanfoo merged commit bea4b32 into zarr-developers:master Nov 16, 2018
@alimanfoo
Copy link
Member Author

Thanks @jakirkham.

@alimanfoo alimanfoo deleted the msgpack-alimanfoo-20181106 branch November 16, 2018 17:14
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.

msgpack: PendingDeprecationWarning: encoding is deprecated, Use raw=False instead
2 participants