Skip to content

Conversation

dstansby
Copy link
Contributor

This makes ObjectStore generic with respect to the underlying store, mirroring similar structure in LoggingStore and WrapperStore. This allows type checkers to infer the type of the underlying store:

from typing import reveal_type

import obstore.store

from zarr.storage import ObjectStore

store = ObjectStore(obstore.store.MemoryStore())
reveal_type(store.store)
# Before: Revealed type is "Union[obstore.store.AzureStore, obstore.store.GCSStore, obstore.store.HTTPStore, obstore.store.S3Store, obstore.store.LocalStore, obstore.store.MemoryStore]"
# After:  Revealed type is "obstore.store.MemoryStore"

@github-actions github-actions bot added the needs release notes Automatically applied to PRs which haven't added release notes label Sep 23, 2025
@d-v-b
Copy link
Contributor

d-v-b commented Sep 23, 2025

cc @kylebarron

Copy link

codecov bot commented Sep 23, 2025

Codecov Report

❌ Patch coverage is 14.28571% with 6 lines in your changes missing coverage. Please review.
✅ Project coverage is 61.20%. Comparing base (48e7f4d) to head (ea18a1b).

Files with missing lines Patch % Lines
src/zarr/storage/_obstore.py 14.28% 6 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3486      +/-   ##
==========================================
- Coverage   61.21%   61.20%   -0.01%     
==========================================
  Files          84       84              
  Lines        9923     9924       +1     
==========================================
  Hits         6074     6074              
- Misses       3849     3850       +1     
Files with missing lines Coverage Δ
src/zarr/storage/_obstore.py 60.82% <14.28%> (-0.29%) ⬇️
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@kylebarron
Copy link
Contributor

This is fine code-wise but why do you need to go back to the underlying object store once you've constructed the zarr store?

@dstansby
Copy link
Contributor Author

dstansby commented Sep 23, 2025

I'm not 100% sure, but at least in the tests here in zarr-python the underlying store is accessed. My motiviation for this was fixing typing in our test suite here, so there may well not be a real world use case. Definitely happy to be guided by those who use/develop ObjectStore on whether this change is worth making or not.

As a related aside, if there is no need for users to access the underlying store would it be worth deprecating .store and renaming to ._store to signal that they shouldn't need to access it?

@kylebarron
Copy link
Contributor

I think it's fine to add the generic. I don't think it exposes too much internal details.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs release notes Automatically applied to PRs which haven't added release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants