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

remove implicit groups #292

Merged
merged 5 commits into from
Aug 26, 2024
Merged

Conversation

d-v-b
Copy link
Contributor

@d-v-b d-v-b commented Apr 12, 2024

this PR removes support for implicit groups (Zarr groups with no metadata documentation) from the spec. See #291 for the motivation. Besides changing the content of the text, I don't know what else to change, so I'm keeping this a draft for now.

A mergeable version of this PR would include a section explaining what implicit groups are, why they were added previously, and why they are now removed. I am happy to add this if it looks like this PR has legs.

Related discussion: #184

@MSanKeys963
Copy link
Member

@d-v-b, is this PR ready for review?

@d-v-b
Copy link
Contributor Author

d-v-b commented May 23, 2024

@MSanKeys963 yes!

@d-v-b d-v-b marked this pull request as ready for review May 23, 2024 13:03
@d-v-b
Copy link
Contributor Author

d-v-b commented Jul 23, 2024

how can we move forward with this? I think it's important to make these changes quickly, before we get skew between implementations.

@jhamman
Copy link
Member

jhamman commented Aug 21, 2024

@zarr-developers/steering-council - this PR has sat without review for 4 months. Can we move it forward please?

jbms
jbms previously approved these changes Aug 21, 2024
rabernat
rabernat previously approved these changes Aug 21, 2024
Copy link
Contributor

@rabernat rabernat left a comment

Choose a reason for hiding this comment

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

Thanks for the push @jhamman.

@joshmoore
Copy link
Member

@d-v-b: do you want to take a look at the conflict?

@d-v-b d-v-b dismissed stale reviews from rabernat and jbms via 33268dc August 22, 2024 09:33
@d-v-b
Copy link
Contributor Author

d-v-b commented Aug 22, 2024

@d-v-b: do you want to take a look at the conflict?

done!

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.

Thanks, @d-v-b.

Merging as an agreed upon "adjustment" of v3 needed to a "spec bug" introduced by removing the concept of a root directory without re-evaluating the explicit/implicit concept.

This is a change that all implementors need to be aware of. See the list of "Changes after Provisional Acceptance" for other such items.

cc: @zarr-developers/implementation-council

@joshmoore joshmoore merged commit c2f2966 into zarr-developers:main Aug 26, 2024
1 check passed
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.

6 participants