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

What is the abstract ArrayMetadata class for #2300

Closed
d-v-b opened this issue Oct 2, 2024 · 3 comments
Closed

What is the abstract ArrayMetadata class for #2300

d-v-b opened this issue Oct 2, 2024 · 3 comments

Comments

@d-v-b
Copy link
Contributor

d-v-b commented Oct 2, 2024

Can someone explain why we need an abstract base class for array metadata, as opposed to handling the union of the two distinct v2 array metadata and v3 array metadata classes?

I'm asking for academic reasons, because the things we are modelling (zarr metadata documents) are not, in fact, hierarchically related, and we don't anticipate any new metadata documents coming on the scene any time soon. Modelling two similar, but importantly distinct entities as a union seems simpler to me than imposing a common API atop them.

I'm also asking for practical reasons, because I have a forthcoming PR where I have to prune a property off the ArrayMetadata abc to accurately model v2 metadata, and it made me wonder how load bearing that abc really is.

@TomAugspurger
Copy link
Contributor

as opposed to handling the union of the two distinct v2 array metadata and v3 array metadata classes?

FWIW, I've wondered the same thing :)

I think that defining ArrayMetadata = ArrayV2Metadata | ArrayV3Metadata is simpler and lets us write code that's generic over the zarr format equally well.

@normanrz
Copy link
Contributor

normanrz commented Nov 6, 2024

I think that defining ArrayMetadata = ArrayV2Metadata | ArrayV3Metadata is simpler and lets us write code that's generic over the zarr format equally well.

That is what we had initially. @jhamman do you remember why we switched to an abc?

@d-v-b
Copy link
Contributor Author

d-v-b commented Nov 6, 2024

I think this issue can be closed, since ArrayMetadata is gone as of #2301

@d-v-b d-v-b closed this as completed Nov 6, 2024
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

No branches or pull requests

3 participants