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.

Sign up for GitHub

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

Jump to bottom

Stateful store tests #2070

Merged
merged 52 commits into from
Aug 15, 2024
Merged

Stateful store tests #2070

merged 52 commits into from
Aug 15, 2024

Conversation

e-marshall
Copy link
Contributor

@e-marshall e-marshall commented Aug 9, 2024 •
edited
Loading

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)

dcherian
dcherian reviewed Aug 9, 2024
View reviewed changes
src/zarr/strategies.py Outdated Show resolved Hide resolved
dcherian
dcherian reviewed Aug 9, 2024
View reviewed changes
src/zarr/strategies.py Outdated Show resolved Hide resolved
@jhamman jhamman added the V3 Related to compatibility with V3 spec label Aug 9, 2024
@e-marshall @dcherian
Update src/zarr/strategies.py
15adda3 
Co-authored-by: Deepak Cherian <dcherian@users.noreply.github.com>
dcherian
dcherian approved these changes Aug 14, 2024
View reviewed changes
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
jhamman
jhamman reviewed Aug 14, 2024
View reviewed changes
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.

tests/v3/test_store/test_stateful_store.py Outdated Show resolved Hide resolved
tests/v3/test_store/test_stateful_store.py Outdated Show resolved Hide resolved
tests/v3/test_store/test_stateful_store.py Outdated Show resolved Hide resolved
tests/v3/test_store/test_stateful_store.py Outdated Show resolved Hide resolved
tests/v3/test_store/test_stateful_store.py Outdated Show resolved Hide resolved
tests/v3/test_store/test_stateful_store.py Show resolved Hide resolved
tests/v3/test_store/test_stateful_store.py Outdated Show resolved Hide resolved
tests/v3/test_store/test_stateful_store.py Outdated Show resolved Hide resolved
@e-marshall @jhamman
Apply suggestions from code review
28811db 
Co-authored-by: Joe Hamman <jhamman1@gmail.com>
@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

jhamman
jhamman approved these changes Aug 15, 2024
View reviewed changes
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. 🙌

tests/v3/test_store/test_stateful_store.py Outdated Show resolved Hide resolved
tests/v3/test_store/test_stateful_store.py Outdated Show resolved Hide resolved
tests/v3/test_store/test_stateful_store.py Outdated Show resolved Hide resolved
tests/v3/test_store/test_stateful_store.py Outdated Show resolved Hide resolved
tests/v3/test_store/test_stateful_store.py Outdated Show resolved Hide resolved
tests/v3/test_store/test_stateful_store.py Outdated Show resolved Hide resolved
e-marshall and others added 2 commits August 14, 2024 21:39
@e-marshall @jhamman
Apply suggestions from code review
622d685 
Co-authored-by: Joe Hamman <jhamman1@gmail.com>
@e-marshall
add review comments
8d2780a
@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 •
edited
Loading

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
Reviewers

@dcherian dcherian dcherian approved these changes

@jhamman jhamman jhamman approved these changes

Assignees
No one assigned
Labels
V3 Related to compatibility with V3 spec
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@e-marshall @dcherian @d-v-b @jhamman