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

feature(store): list_* -> AsyncGenerators #1844

Merged
merged 5 commits into from
May 7, 2024
Merged

feature(store): list_* -> AsyncGenerators #1844

merged 5 commits into from
May 7, 2024

Conversation

jhamman
Copy link
Member

@jhamman jhamman commented May 7, 2024

This PR peels a few bits off from #1743, primarily focused on moving the output of the store list_* methods to AsyncGenerators.

A few other nice things that ended up here:

  • add zarr.testing.store module to support downstream use cases
  • set pytest-asyncio mode to auto

@jhamman
feature(store): list_* -> AsyncGenerators
0e035fb 
- add zarr.testing.store module to support downstream use cases
- set pytest-asyncio mode to auto
-
@jhamman jhamman added the V3 Affects the v3 branch label May 7, 2024
@jhamman
Copy link
Member Author

jhamman commented May 7, 2024

@dstansby - wondering if you can help me debug these mypy failures:

Warning: --strict-concatenate is deprecated; use --extra-checks instead
src/zarr/testing/store.py:9: error: Untyped decorator makes function "store" untyped  [misc]
src/zarr/testing/store.py:25: error: Untyped decorator makes function "test_set_get_bytes_roundtrip" untyped  [misc]
src/zarr/testing/store.py:26: error: Untyped decorator makes function "test_set_get_bytes_roundtrip" untyped  [misc]
src/zarr/testing/store.py:31: error: Untyped decorator makes function "test_get_partial_values" untyped  [misc]
src/zarr/testing/store.py:32: error: Untyped decorator makes function "test_get_partial_values" untyped  [misc]

I'm really surprised this isn't well supported by mypy/pytest. 🤷

jhamman
jhamman commented May 7, 2024
View reviewed changes
src/zarr/testing/store.py
@@ -0,0 +1,81 @@
import pytest
Copy link
Member Author

@jhamman jhamman May 7, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

The idea here is that we will provide a test harness for the generic store interface. This doesn't mean we can't have store-specific tests but those can go in our tests directory.

Importing zarr.testing.store will raise an ImportError if pytest is not installed. I think that could be fine but calling it out so others have a chance to comment. I think there are some things we can do to further hide this import if folks object to the current configuration.

jhamman
jhamman commented May 7, 2024
View reviewed changes
tests/v2/test_storage.py
@@ -1,96 +1,87 @@
# import array
import array
Copy link
Member Author

@jhamman jhamman May 7, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

The story here is that when we first started the v3 work, I tried to "reuse" the v2 store tests. It wasn't a great fit and I wasn't able to migrate many tests to the new API those changes rather than continuing to use this module.

The contents match this file in test_storage.py at the time we created the v3 branch (minus the v3 specific bits that have since been removed).

@jhamman jhamman added this to the 3.0.0.alpha milestone May 7, 2024
@jhamman jhamman mentioned this pull request May 7, 2024
Closed
4 tasks
@jhamman jhamman force-pushed the feature/store-list-async-gen branch from f0c57cb to eb49dd6 Compare May 7, 2024 05:56
@dstansby dstansby mentioned this pull request May 7, 2024
Merged
6 tasks
@dstansby
Copy link
Contributor

dstansby commented May 7, 2024

@dstansby - wondering if you can help me debug these mypy failures:

Warning: --strict-concatenate is deprecated; use --extra-checks instead
src/zarr/testing/store.py:9: error: Untyped decorator makes function "store" untyped  [misc]
src/zarr/testing/store.py:25: error: Untyped decorator makes function "test_set_get_bytes_roundtrip" untyped  [misc]
src/zarr/testing/store.py:26: error: Untyped decorator makes function "test_set_get_bytes_roundtrip" untyped  [misc]
src/zarr/testing/store.py:31: error: Untyped decorator makes function "test_get_partial_values" untyped  [misc]
src/zarr/testing/store.py:32: error: Untyped decorator makes function "test_get_partial_values" untyped  [misc]

I'm really surprised this isn't well supported by mypy/pytest. 🤷

I think #1846 will fix this

d-v-b
d-v-b approved these changes May 7, 2024
View reviewed changes
Copy link
Contributor

@d-v-b d-v-b left a comment

Choose a reason for hiding this comment

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

looks good, thanks!!

@jhamman jhamman merged commit 3499acb into zarr-developers:v3 May 7, 2024
18 checks passed
@jhamman
Copy link
Member Author

jhamman commented May 7, 2024

Thanks @d-v-b for the review and @dstansby for fixing the mypy thing.

@jhamman jhamman deleted the feature/store-list-async-gen branch May 7, 2024 16:26
@DimitriPapadopoulos DimitriPapadopoulos mentioned this pull request Oct 16, 2024
Merged
6 tasks
@DimitriPapadopoulos DimitriPapadopoulos mentioned this pull request Nov 5, 2024
Merged
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@d-v-b d-v-b d-v-b approved these changes

Assignees
No one assigned
Labels
V3 Affects the v3 branch
Projects
Zarr-Python - 3.0
Status: Done
Milestone
3.0.0.alpha
Development

Successfully merging this pull request may close these issues.
3 participants
@jhamman @dstansby @d-v-b