Codecov Report

❌ Patch coverage is72.22222% with15 lines in your changes missing coverage. Please review.
✅ Project coverage is 61.21%. Comparing base (62551c7) to head (c8157fd).
⚠️ Report is 2 commits behind head on main.

@@            Coverage Diff             @@##             main    #3407      +/-   ##==========================================- Coverage   61.24%   61.21%   -0.04%==========================================  Files          83       84       +1       Lines        9907     9923      +16     ==========================================+ Hits         6068     6074       +6- Misses       3839     3849      +10

conceptually I don't think we want azarr.core.metadata.io module. So far, thezarr.core.metadata module has been exclusively for definitions of the metadata classes, which is distinct from the routines for saving metadata documents to disk.

right now I think a better location would bezarr.core.group -- would that be possible?

Thanks@d-v-b - I think the only issue with this is it may introduce a circular import?zarr.core.group already imports multiple functions fromzarr.core.array, andzarr.core.array would need to import these common metadata saving functions fromzarr.core.group.

I could move those functions intozarr.core.array instead?_build_parents used to be in that file, and was imported by bothzarr.core.array andzarr.core.group (although only by putting an import inside that function to avoid another circular import 😅 )

very good points@K-Meech. Now I do think we should havezarr.core.metadata.io. But I don't think it should depend onAsyncArray orAsyncGroup. Looking at_build_parents, all it does is save metadata. I don't think this class needs to useAsyncGroup for that, we can just write the metadata out directly without that class.

In fact, we might not need the metadata-saving logic in_build_parents, since it's being called byanother routine that's already creating metadata (save_metadata). IMO_build_parents should just decide where metadata documents need to be created, and then return a{name: GroupMetadata} dict with the names of the groups that need to be created by the calling function (e.g.,save_metadata).

I suspect all of this could be handled bycreate_hierarchy. In any case, that function should also be moved tozarr.core.metadata.io (i'm happy to do this in another PR if you don't want to)

edit: I mistakenly linked to thecreate_hierarchy method defined on theAsyncGroup class. I meant to link to the stand-alone function:

Thanks@d-v-b - sounds like a good plan. I'll look into removing the dependency onAsyncArray +AsyncGroup.
I'm away for the next few weeks, but can look into this + any other comments left on the PR in the meantime once I'm back.

@d-v-b I've updated the code now to match your suggestion above. The dependency onAsyncArray /AsyncGroup is removed, and_build_parents now returns a dict of{name: GroupMetadata} thatsave_metadata then creates.

I agree thatcreate_hierarchy could probably handle much of this functionality, but perhaps it's best to change this in a separate PR to keep the changes minimal here? What do you think?

I agree thatcreate_hierarchy could probably handle much of this functionality, but perhaps it's best to change this in a separate PR to keep the changes minimal here? What do you think?

That works for me!