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

functions to get children of Group #104

Merged
merged 10 commits into from
Dec 25, 2024
Merged

functions to get children of Group #104

merged 10 commits into from
Dec 25, 2024

Conversation

niklasmueboe
Copy link
Contributor

First draft of functions to directly get the children of a Group.

Not sure what the API should look like. One could also consider returning a list of Nodes which correspond to only Arrays/Groups instead of returning a list of Arrays/Groups directly for the function child_arrays/child_groups, respectively.

Async function could also be added if needed.

Functions that only return a list of child names (i.e. Vec<String>) instead of Arrays/Groups could also be useful. Something akin to https://zarr.readthedocs.io/en/stable/api/hierarchy.html#zarr.hierarchy.Group.group_keys

Would be good to get some feedback, then I can make some further adjustments.

Disclaimer; I am still quite new to Rust.

Copy link

codecov bot commented Dec 21, 2024

Codecov Report

Attention: Patch coverage is 9.00000% with 91 lines in your changes missing coverage. Please review.

Project coverage is 81.72%. Comparing base (9a4c540) to head (cee7dda).
Report is 13 commits behind head on main.

Files with missing lines Patch % Lines
zarrs/src/group.rs 0.00% 77 Missing ⚠️
zarrs/src/node/node_async.rs 0.00% 10 Missing ⚠️
zarrs/src/node.rs 0.00% 3 Missing ⚠️
zarrs/src/node/node_sync.rs 90.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #104      +/-   ##
==========================================
- Coverage   81.97%   81.72%   -0.26%     
==========================================
  Files         166      166              
  Lines       23489    23578      +89     
==========================================
+ Hits        19255    19268      +13     
- Misses       4234     4310      +76     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@LDeakin
Copy link
Owner

LDeakin commented Dec 21, 2024

Thanks @niklasmueboe! The API looks good.

A couple of suggestions:

  • Use get_direct_child_nodes instead of get_child_nodes in child_arrays / child_groups, since you don't need to resolve all children recursively
  • Create groups / arrays with new_with_metadata to avoid parsing the metadata twice

zarrs/src/group.rs Outdated Show resolved Hide resolved
@niklasmueboe
Copy link
Contributor Author

What do you think of the recursive option for listing the children? Alternatively, it could also be split into separate methods, i.e. [direct_]children, [direct_]child_groups.

Should these methods return a Result<Vec<Group>, _> or Iterator with Result<Group, _> Items (and correspondingly for Array)?

@LDeakin LDeakin marked this pull request as ready for review December 24, 2024 23:57
@LDeakin LDeakin self-requested a review as a code owner December 24, 2024 23:57
Copy link
Owner

@LDeakin LDeakin left a comment

Choose a reason for hiding this comment

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

Great stuff @niklasmueboe!

What do you think of the recursive option for listing the children

I like it, and subsequently I have removed the recently added get_direct_child_nodes in favour of adding a recursive parameter to get_child_nodes, but that will have to wait for the next breaking release.

I hope you don't mind me just finishing this PR off, I'll get a new release out later today with these changes.

@LDeakin LDeakin merged commit 3c6e041 into LDeakin:main Dec 25, 2024
22 checks passed
@LDeakin
Copy link
Owner

LDeakin commented Dec 25, 2024

Functions that only return a list of child names (i.e. Vec) instead of Arrays/Groups could also be useful.

+1. Feel free to raise additional PRs if you can think of any other useful additions to this library.

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