-
Notifications
You must be signed in to change notification settings - Fork 29
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
Apply more ZEP 1 feedback #171
Apply more ZEP 1 feedback #171
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is looking good.
I noted an inconsistency in how we are identifying the root node.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is looking good.
I noted an inconsistency in how we are identifying the root node.
@jbms @rabernat Is there anything else missing IYO? I'd love to merge this soon to avoid conflicts and to have the feedback incorporated into https://zarr-specs.readthedocs.io/en/latest/core/v3.0.html. |
Thanks @rabernat 👍 |
This PR applies review feedback from the ZEP1 review in PR #149. Most changes are clarifications and wording corrections. The only further reaching changes are about
metadata_key_suffix
andmetadata_encoding
, explained below. The changes for those fields are not finalized yet, and should be discussed further in #174, separate from this PR, to unblock the other changes. Sorry about mixing those up. If the related changes do not seem acceptable for the moment, I'd rather fall back to removing them and solve this in a separate issue/PR.Non editorial changes:
metadata_key_suffix
into themetadata_encoding
metadata field, specifying only the encoding schema there, not the metadata content. Changedmetadata_encoding
to be an extension point.