Remove the unused zarrSchema module - #107
Conversation
The v2/v3 `.zarray` zod schemas had no callers and were never exported from the package. Routing the tree builder through them was the alternative considered, and it runs against the decision this package just made: `ZarrArrayMetadata` admits an unrecognised record precisely so a store carrying a data type we do not model still opens, and `getArrayDtype` answers `undefined` for such a type rather than throwing. A schema that rejects the whole store at open time is the opposite of that — and zarr v3 extension dtypes, written as objects rather than strings, are exactly the case it would have rejected. `zarrita` already validates array metadata on the real read path. The one useful thing the module did — reconciling v2's `dtype` with v3's `data_type` — is now `normalizeDtype`, which does it in `zarrita`'s own `DataType` vocabulary rather than leaving a numpy typestring sitting in a v3-shaped `data_type` field, as `validateAndConvertV2Zarray` did. `ZarrV3ArrayNode` stays: it is exported, and describes metadata as it arrives from a store rather than anything the deleted validator produced. Its docstring pointed at `validateV3Zarray` for that contrast, so it now points at `ZarrArrayMetadata`, where the rationale lives. Drops `zod` from the package, which nothing else in it imports. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`ZarrV3GroupNode` and `ZarrV3Metadata` describe zarr.json's group and consolidated-metadata shapes, but nothing reads them: they reference only each other, are not exported from index.ts, and types.ts is not reachable through the package's exports map. The tree builder does not use them either — `parseStoreContents` works from zarrita's `store.contents()` rather than from a typed view of zarr.json. They predate the zarrSchema removal rather than being orphaned by it; they are going now only because that pass put the file under the light. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Warning Review limit reached
Next review available in: 50 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe change removes unused Zarr schema validation and its tests, removes the ChangesZarr metadata cleanup
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.changeset/hungry-donkeys-shave.md:
- Line 7: Update the changeset wording to identify the metadata filenames
accurately: v2 uses `.zarray`, while v3 uses `zarr.json`; alternatively, refer
generically to v2/v3 array metadata schemas without implying both use `.zarray`.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: bbed198b-f59c-46dc-8b45-ec6a3f955af9
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (5)
.changeset/hungry-donkeys-shave.mdpackages/zarrextra/package.jsonpackages/zarrextra/src/types.tspackages/zarrextra/src/zarrSchema.tspackages/zarrextra/tests/zarrSchema.spec.ts
💤 Files with no reviewable changes (2)
- packages/zarrextra/tests/zarrSchema.spec.ts
- packages/zarrextra/src/zarrSchema.ts
v2 writes `.zarray` and v3 writes `zarr.json` — `metadataKeysForPath` reads both, and the `ZarrV2ArrayNode` / `ZarrV3ArrayNode` docstrings in the same package already say so. The changeset said "v2/v3 `.zarray`", which implies v3 uses `.zarray` too. Changesets become published CHANGELOG entries, so the wording is worth getting right. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Nothing here was exported, so the only thing a `zarrextra` consumer can observe is `zod` leaving the dependency list. The argument for deleting rather than wiring the schemas into the tree builder is review material, and already lives in e400aee and the PR — a published CHANGELOG entry is the wrong place for it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Follows #105.
packages/zarrextra/src/zarrSchema.tshad no callers: not exported fromindex.ts, referenced nowhere outside its own spec.The choice was to either route the tree builder's array-metadata read through these schemas — so
parseStoreContents→readMetadataJsonvalidates at the boundary instead of storing a bareJSON.parseresult underZARRAY_KEY— or delete the module. This does the latter.Why not validate on open
#105 already decided this, and wrote the decision down. From
ZarrArrayMetadata's docstring:A schema that rejects a store at open time is that same failure by another route. The module's own comments already recorded it happening —
stringis absent from the v3 spec's data-type listing, so the schema failed on the first real object it met, and thev3dtypeSchemaliteral union still models no extension dtypes at all.Validation here would also be redundant twice over.
zarritavalidates array metadata on the real read path (zarr.open), and the tree'sZARRAY_KEYcopy is a metadata cache, not the thing chunks are read through.The one genuinely useful thing the module did — reconciling v2's
dtypewith v3'sdata_type— isnormalizeDtype/getArrayDtypeas of #105, and they do it better: inzarrita's ownDataTypevocabulary, returningundefinedfor an unmodelled type rather than throwing.validateAndConvertV2Zarrayclaimed aZarrV3ArrayNodereturn while leaving the raw numpy typestring (<f8) sitting indata_type, so wiring it in would have put a v2 value in a v3-shaped field — exactly the ambiguitygetArrayDtypeexists to remove.Conversion was lossy besides: it hardcoded
attributes: {}, discarding attrs the tree keeps; forcedzarr_format: 3; and synthesisedchunk_key_encodingwith separator/, where v2's default is..Changes
zarrSchema.ts(303 lines) andzarrSchema.spec.ts(264 lines)ZarrV3ArrayNode's docstring, which referenced{@link validateV3Zarray}for the validated/unvalidated contrast, atZarrArrayMetadatawhere that rationale now livesZarrV3GroupNodeandZarrV3Metadata— referenced only by each other, not exported fromindex.ts, andtypes.tsis unreachable through the package'sexportsmap. These predate zarrextra: runtime type guards for tree nodes, docs, and build/test packaging fixes #105; they go here only because this pass put the file under the light.zodfromzarrextra, which nothing else in the package importsZarrV3ArrayNodeandZarrV2ArrayNodestay — both are exported, and are the members of the publicZarrArrayMetadataunion.Net: 12 insertions, 605 deletions. No public API removed: nothing deleted here was reachable from
zarrextra's entry point.Verification
pnpm build,pnpm test:unit(795 passed / 96 files),pnpm lint:biome, and the docs build all pass on the rebased branch. Patch changeset included;zarrextratakes a single patch bump alongside #105's still-pendingzarr-tree-node-guards.🤖 Generated with Claude Code
Summary by CodeRabbit
Refactor
Documentation