Skip to content

Stateful store tests #2070

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

Merged
merged 52 commits into from
Aug 15, 2024
Merged

Conversation

e-marshall
Copy link
Contributor

@e-marshall e-marshall commented Aug 9, 2024

This is a PR that implements stateful tests for Zarr MemoryStores.

TODO items below:

  • I added docstrings to new classes, functions
  • I don't think any changes need to be made to tutorial.rst
  • I wasn't sure if these changes should be added in release.rst ?
  • GH actions pass
  • I wasn't sure where to find codecov results, but didn't see a notification that it failed

TODO:

  • Add unit tests and/or doctests in docstrings
  • Add docstrings and API docs for any new/modified user-facing classes and functions
  • New/modified features documented in docs/tutorial.rst
  • Changes documented in docs/release.rst
  • GitHub Actions have all passed
  • Test coverage is 100% (Codecov passes)

@jhamman jhamman added the V3 label Aug 9, 2024
Co-authored-by: Deepak Cherian <[email protected]>
Copy link
Contributor

@dcherian dcherian left a comment

Choose a reason for hiding this comment

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

Very nice work, Emma!

👏🏾 👏🏾 👏🏾

@e-marshall e-marshall marked this pull request as ready for review August 14, 2024 04:05
@e-marshall
Copy link
Contributor Author

thanks @dcherian ! thanks for all your help, fun to work with you and Seba on this!

@e-marshall e-marshall marked this pull request as draft August 14, 2024 16:57
@e-marshall e-marshall marked this pull request as ready for review August 14, 2024 19:18
Copy link
Member

@jhamman jhamman left a comment

Choose a reason for hiding this comment

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

Super work @e-marshall! I have a bunch of comments on the Store wrapper -- none of them major.

@e-marshall
Copy link
Contributor Author

thanks @jhamman ! I made these changes on d63df09. happy to tweak/shorten the statemachine docstring if it seems too long

Copy link
Member

@jhamman jhamman left a comment

Choose a reason for hiding this comment

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

One last set of small comments but I'm approving this as is. 🙌

@e-marshall
Copy link
Contributor Author

thanks @jhamman !

@dcherian
Copy link
Contributor

Can we merge so that one might add list_prefix? #2064

@d-v-b
Copy link
Contributor

d-v-b commented Aug 15, 2024

Can we merge so that one might add list_prefix? #2064

I am working on getting that PR updated and ready

@dcherian
Copy link
Contributor

dcherian commented Aug 15, 2024

Right, I meant that we could merge this PR so that we can add list_prefix to the stateful tests in that PR

@jhamman jhamman merged commit 681f286 into zarr-developers:v3 Aug 15, 2024
25 checks 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.

4 participants