Skip to content

add modules for types #3355

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
Open

Conversation

d-v-b
Copy link
Contributor

@d-v-b d-v-b commented Aug 6, 2025

This does a few things:

  • collects a lot of type definitions and constants in zarr.core.types,
  • exports some of those types from zarr.types
  • defines complete typeddict models for zarr v2 and zarr v3 metadata documents. These are useful enough on their own that we should add them to the library even if this PR goes nowhere.

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

codecov bot commented Aug 6, 2025

Codecov Report

❌ Patch coverage is 97.19626% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 94.54%. Comparing base (a26926c) to head (3e119a9).

Files with missing lines Patch % Lines
src/zarr/types.py 0.00% 2 Missing ⚠️
src/zarr/core/common.py 75.00% 1 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main    #3355   +/-   ##
=======================================
  Coverage   94.54%   94.54%           
=======================================
  Files          78       80    +2     
  Lines        9423     9467   +44     
=======================================
+ Hits         8909     8951   +42     
- Misses        514      516    +2     
Files with missing lines Coverage Δ
src/zarr/abc/codec.py 94.28% <100.00%> (ø)
src/zarr/abc/metadata.py 95.65% <ø> (ø)
src/zarr/abc/store.py 95.80% <ø> (ø)
src/zarr/api/asynchronous.py 87.62% <ø> (ø)
src/zarr/api/synchronous.py 92.95% <ø> (ø)
src/zarr/codecs/blosc.py 88.11% <100.00%> (ø)
src/zarr/codecs/bytes.py 96.87% <100.00%> (ø)
src/zarr/codecs/crc32c_.py 97.05% <100.00%> (ø)
src/zarr/codecs/gzip.py 91.42% <100.00%> (ø)
src/zarr/codecs/sharding.py 94.49% <100.00%> (+0.01%) ⬆️
... and 44 more
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

ianhi added a commit to ianhi/zarr-python that referenced this pull request Aug 8, 2025
…ure with comprehensive exports

This approach builds on PR zarr-developers#3355's zarr.core.types foundation while massively
expanding zarr.types public exports to enable comprehensive type documentation
linking. The hybrid strategy provides the best of both worlds:

- Maintains clean architectural separation (internal zarr.core.types vs public zarr.types)
- Enables comprehensive documentation coverage (~75% vs ~7% with vanilla PR zarr-developers#3355)
- Provides excellent developer experience with single import location
- Supports all common types used in function signatures for proper Sphinx linking

Key additions to zarr.types public API:
- Core classes: Array, AsyncArray, Group, AsyncGroup, ZDType, DefaultFillValue
- Store types: Store, StorePath, StoreLike, ByteGetter, ByteSetter
- Array types: NDArrayLike, ArrayLike, ZDTypeLike
- Metadata types: ArrayV2Metadata, ArrayV3Metadata, GroupMetadata, ConsolidatedMetadata
- Codec types: BaseCodec, ArrayArrayCodec, BytesBytesCodec, CompressorLike, etc.
- Configuration types: ArrayConfig, ArrayConfigLike, ChunkKeyEncodingLike
- Enum types: BloscCname, BloscShuffle, Endian, Order, etc.

This enables the core goal: "ideally everything in a type hint will be linkable"
while maintaining good software engineering practices.

🤖 Generated with [Claude Code](https://claude.ai/code)

Co-Authored-By: Claude <[email protected]>
Co-Authored-By: Davis Bennett <[email protected]>
@dstansby
Copy link
Contributor

The trouble with defining objects in two places (here, zarr.types and zarr.core.types) is duplication, and also means we end up importing stuff in the tests from the private place (zarr.core.types here) instead of the public place (zarr.types here), which in the past has prevented us from noticing a change in the public API (zarr.types here) because we're not actually using it in any of our tests.

To prevent this, could we either:

  1. Make all the types public and just put them in zarr.types
  2. Put all types in zarr.types, but make some private with a leading underscore in their name

?

Related, there are types that are in zarr.core.types that are used in the public API (at least ShapeLike, I'm sure many others too). If a type is used in the public API, it should be publicly documented, so there needs to be re-evaluation of what is public/private types here.

@d-v-b
Copy link
Contributor Author

d-v-b commented Aug 13, 2025

Related, there are types that are in zarr.core.types that are used in the public API (at least ShapeLike, I'm sure many others too). If a type is used in the public API, it should be publicly documented, so there needs to be re-evaluation of what is public/private types here.

that might be the first step to refining this PR -- we should figure out exactly which types are public. Definitely anything used as an annotation of a public function signature should be public. In this case, these types should be defined just once in zarr.types. All other private types can be defined wherever is convenient for them, which could be zarr.core.types but we could also have private, dtype-specific types in zarr.core.dtype, for example

@d-v-b
Copy link
Contributor Author

d-v-b commented Aug 13, 2025

oops, I forgot that #3304 also adds a types module. ignore this PR until #3304 is sorted out

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.

2 participants