Skip to content

Refactor out _tofile/_fromfile from DirectoryStore #503

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Merged
merged 7 commits into from
Nov 12, 2019
Merged

Refactor out _tofile/_fromfile from DirectoryStore #503

merged 7 commits into from
Nov 12, 2019

Conversation

jakirkham
Copy link
Member

To make it easier for users to implement their own methods for reading and writing from disk ( like memory-mapping #377 ), refactor out _tofile and _fromfile methods that are called to handle the reading and writing of the value. Should avoid duplicating a lot of other logic when trying to extend DirectoryStores for these cases.

TODO:

  • Add unit tests and/or doctests in docstrings
  • Add docstrings and API docs for any new/modified user-facing classes and functions
  • New/modified features documented in docs/tutorial.rst
  • Changes documented in docs/release.rst
  • Docs build locally (e.g., run tox -e docs)
  • AppVeyor and Travis CI passes
  • Test coverage is 100% (Coveralls passes)

To make it easier to sub out different ways of reading and writing data,
create to methods to handle reading and writing directly. This way users
don't need to reimplement all of the same logic that `__getitem__` and
`__setitem__` are doing and can focus on simply reading and writing. Can
be useful for trying memmaping for example.
Co-Authored-By: James Bourbeau <jrbourbeau@users.noreply.github.com>
Copy link
Member

@mzjp2 mzjp2 left a comment

Choose a reason for hiding this comment

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

LGTM! 🎉 I'm missing context on this, but is it worth making mode a keyword argument which defaults to rb/wb or is there no situation where someone might need a different mode?

@jakirkham
Copy link
Member Author

The idea would be subclasses could override _tofile and _fromfile. So they could change modes or even the way the file is written/read altogether.

Copy link
Member

@jrbourbeau jrbourbeau left a comment

Choose a reason for hiding this comment

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

Thanks @jakirkham, this looks great and should definitely help with extending the DirectoryStore! I've left one small documentation comment below. Also, could you please add a changelog entry? Otherwise LGTM

@jakirkham
Copy link
Member Author

Thanks for the feedback. Have added two docstrings based on what you mentioned. Please let me know if there is anything else. 🙂

@jrbourbeau jrbourbeau mentioned this pull request Nov 11, 2019
7 tasks
@jakirkham
Copy link
Member Author

Planning on merging EOD tomorrow if no comments.

Copy link
Member

@jrbourbeau jrbourbeau left a comment

Choose a reason for hiding this comment

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

Thanks @jakirkham!

@jrbourbeau jrbourbeau merged commit 8721a52 into zarr-developers:master Nov 12, 2019
@jakirkham jakirkham deleted the ref_tofile_fromfile branch November 12, 2019 03:22
@jakirkham
Copy link
Member Author

Thanks James! 😄

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants