Skip to content

Expose LatencyStore as public API #3359

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

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

TomNicholas
Copy link
Member

@TomNicholas TomNicholas commented Aug 8, 2025

Closes ##3358

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/user-guide/*.rst
  • Changes documented as a new file in changes/
  • GitHub Actions have all passed
  • Test coverage is 100% (Codecov passes)

@github-actions github-actions bot added the needs release notes Automatically applied to PRs which haven't added release notes label Aug 8, 2025
@github-actions github-actions bot removed the needs release notes Automatically applied to PRs which haven't added release notes label Aug 8, 2025
Copy link

codecov bot commented Aug 8, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 94.54%. Comparing base (926a52f) to head (eb4aa28).

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #3359   +/-   ##
=======================================
  Coverage   94.54%   94.54%           
=======================================
  Files          78       79    +1     
  Lines        9423     9427    +4     
=======================================
+ Hits         8909     8913    +4     
  Misses        514      514           
Files with missing lines Coverage Δ
src/zarr/storage/__init__.py 95.23% <100.00%> (+0.23%) ⬆️
src/zarr/storage/_latency.py 100.00% <100.00%> (ø)
src/zarr/testing/store.py 100.00% <ø> (ø)
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@d-v-b
Copy link
Contributor

d-v-b commented Aug 8, 2025

pre-commit is mad about a docstring but otherwise this looks g2g

@maxrjones
Copy link
Member

I'm a bit confused about why this is necessary. It's extremely common for downstream libraries to use functions from the testing modules of libraries (e.g., np.testing.assert_..., xr.testing.assert_...). Why do we need a different approach here? I question whether this change is worth it because putting LatencyStore in the same namespace at the other stores would add confusion for people who just want to use Zarr-Python, not develop downstream libraries.

@TomNicholas
Copy link
Member Author

TomNicholas commented Aug 9, 2025

Why do we need a different approach here? I question whether this change is worth it because putting LatencyStore in the same namespace at the other stores would add confusion for people who just want to use Zarr-Python, not develop downstream libraries.

I actually agree - I think it would make more sense to expose LatencyStore within a zarr.testing.storage namespace. That still requires a PR though (which could be this one), because it's currently in zarr.testing.store, and not documented in the public API docs.

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.

3 participants