Skip to content

raise an error if create_dataset's dimension_separator is inconsistent with the provided store #724

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

Conversation

grlee77
Copy link
Contributor

@grlee77 grlee77 commented Apr 27, 2021

#Description

While working on the zarr_implementations repository, at one point I generated data with create_dataset using dimension_separator='/', but with the default FSStore which has a key_separator of '.'. This did not result in any warning, but generated a flat storage format despite the specification of dimension_separator. This PR is intended to prevent this by checking for consistency between a store's dimension separator and the one passed as an argument to create_dataset.

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)

@codecov
Copy link

codecov bot commented Apr 27, 2021

Codecov Report

Merging #724 (1769f8e) into master (2d0acfb) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master     #724   +/-   ##
=======================================
  Coverage   99.94%   99.94%           
=======================================
  Files          28       28           
  Lines       10433    10442    +9     
=======================================
+ Hits        10427    10436    +9     
  Misses          6        6           
Impacted Files Coverage Δ
zarr/creation.py 100.00% <100.00%> (ø)
zarr/tests/test_hierarchy.py 100.00% <100.00%> (ø)

@joshmoore
Copy link
Member

Nice! This might be the better idea for #722, rather than trying to propagate the value down into wrapped store.

Copy link
Member

@joshmoore joshmoore left a comment

Choose a reason for hiding this comment

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

👍

@joshmoore joshmoore merged commit 4d2f813 into zarr-developers:master Apr 27, 2021
@joshmoore
Copy link
Member

Released as 2.8.1

@joshmoore joshmoore mentioned this pull request Feb 9, 2022
19 tasks
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