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

Move metadata handling to a class (updated) #839

Merged
merged 51 commits into from
Nov 4, 2021

Conversation

grlee77
Copy link
Contributor

@grlee77 grlee77 commented Sep 24, 2021

Description

This PR is an update of #581 and builds upon #789 (only the last few commits, starting from 0e1fbe8 are new). This PR is not changing existing metadata function behavior, only moving the existing functions to class methods.

The Store from #789 gets a new _metadata_class attribute that other higher level functions can use to call functions that encode/decode metadata. To transition to v3 support, we would just make a new v3 version of this metadata class that overrides encode/decode methods to account for the changes in metadata format.

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
  • GitHub Actions have all passed
  • Test coverage is 100% (Codecov passes)

grlee77 and others added 30 commits June 23, 2021 11:12
add assert statements to test_capabilities
TST: initialize stores with KVStore(dict()) instead of bare dict()
ensures Attributes.store is a Store
ensures open_array, etc can work when the user supplies a dict
want to test the non-Store code path in _ensure_store
@codecov
Copy link

codecov bot commented Sep 24, 2021

Codecov Report

Merging #839 (fff210a) into master (5c71212) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master     #839   +/-   ##
=======================================
  Coverage   99.94%   99.94%           
=======================================
  Files          32       32           
  Lines       11175    11204   +29     
=======================================
+ Hits        11169    11198   +29     
  Misses          6        6           
Impacted Files Coverage Δ
zarr/_storage/store.py 100.00% <100.00%> (ø)
zarr/attrs.py 100.00% <100.00%> (ø)
zarr/core.py 100.00% <100.00%> (ø)
zarr/hierarchy.py 100.00% <100.00%> (ø)
zarr/meta.py 100.00% <100.00%> (ø)
zarr/storage.py 100.00% <100.00%> (ø)
zarr/tests/test_storage.py 100.00% <100.00%> (ø)

grlee77 and others added 16 commits September 24, 2021 14:04
BaseStore does not have the listdir or rmdir methods

cleaned up some type declerations, making sure mypy passes
This is done to ease transition to Zarr v3 support.
When adding v3 support, we can override encode and decode methods to account for changes
in the metadata format.
The classmethods are also exported under the old function names for backwards compatibility.

Co-authored-by: Matthias Bussonier <bussonniermatthias@gmail.com>
Because existing functions allow coerce of dict to store, there are a lot of
hasattr calls here. We can remove these checks if we start enforcing that the
input MUST be a Store.

Use of this _metadata_class will ease the transition to v3
This will make it easier to reuse existing testing code when adding v3 support
use _metadata_class methods
In these cases self._store was created using _ensure_store, so it will always have the attribute
@grlee77 grlee77 force-pushed the metadata-classes-v2 branch from 6ac75bf to 15d4d78 Compare October 21, 2021 00:20
@pep8speaks
Copy link

pep8speaks commented Oct 21, 2021

Hello @grlee77! 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 2021-10-21 19:09:50 UTC

@joshmoore
Copy link
Member

@grlee77 : sorry, but it looks like some process of merging/updating has led to conflicts.

@grlee77
Copy link
Contributor Author

grlee77 commented Oct 21, 2021

should be fixed now (assuming CI passes)

@joshmoore
Copy link
Member

No vetoes during the community call yesterday. Rolling into v2.11.0a2.

@joshmoore joshmoore merged commit 440d097 into zarr-developers:master Nov 4, 2021
@jakirkham jakirkham mentioned this pull request Dec 1, 2021
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.

4 participants