Skip to content

Rewrites the ensure_text implementation#201

Merged
jakirkham merged 3 commits intozarr-developers:masterfrom
jakirkham:rewrite_ensure_text
Nov 3, 2019
Merged

Rewrites the ensure_text implementation#201
jakirkham merged 3 commits intozarr-developers:masterfrom
jakirkham:rewrite_ensure_text

Conversation

@jakirkham
Copy link
Copy Markdown
Member

Borrows the ensure_text_type function's implementation from Zarr and uses it to rewrite the ensure_text function. Basically it makes sure any bytes-like data can be encoded as text with zero-copy. Also avoids getting muddled in Python 2/3 details as well, which should make it easier to maintain.

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)

Borrows the `ensure_text_type` function's implementation from Zarr and
uses it to rewrite the `ensure_text` function. Basically it makes sure
any `bytes`-like data can be encoded as text with zero-copy. Also avoids
getting muddled in Python 2/3 details as well, which should make it
easier to maintain.
@pep8speaks
Copy link
Copy Markdown

pep8speaks commented Oct 24, 2019

Hello @jakirkham! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2019-10-24 02:46:04 UTC

To fix some lint, go ahead and replace `l` with `s` in `ensure_text`.
@jakirkham
Copy link
Copy Markdown
Member Author

Not sure what is up with the coverage report. When drilling down into it, am seeing 100% on the individual files.

@jakirkham jakirkham merged commit 4b7ee9b into zarr-developers:master Nov 3, 2019
@jakirkham jakirkham deleted the rewrite_ensure_text branch November 3, 2019 23:15
This was referenced Nov 3, 2019
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