Make type checking cover tests and match CI - #181
Merged
Conversation
The `types` hatch environment installed only mypy, pydantic and numpy, so zarr, pytest, xarray and dask were all missing. Combined with `ignore_missing_imports = true` this turned them into `Any` and produced a large number of phantom errors, while the mypy that actually gated CI was the pre-commit hook, which checked `src` only with a separately pinned version. Give the `types` environment the `test` dependency group and pin mypy, so it type-checks against the same dependencies the tests run against. Remove the mypy pre-commit hook and add a `typecheck` CI job running `hatch run types:check`, making that command the single entry point for local and CI type checking. `sp-repo-review`'s PC140 is ignored since it hard-requires a mypy pre-commit hook. Fix the errors this surfaced in `tests/`, and two genuine typing bugs in `src/`: - `v2.from_zarr` and `experimental.v2.from_zarr` declared `depth` without a default in their overloads, so the documented one-argument call `from_zarr(group)` did not type-check for callers. - `experimental.v2.GroupSpec.from_flat` accepted only `Mapping[str, ArraySpec | GroupSpec]` while `to_flat` returns `BaseGroupSpec` values, so the round trip did not type-check. Also fixes a test assertion in `experimental/test_v2.py` that compared `spec.filters` (a tuple) against `auto_filters()` (a list), which could only ever have passed when both were `None`. Assisted-by: ClaudeCode:claude-opus-5
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
🤖 AI text below 🤖
Splits the type-checking work out of #180 so the
justfilePR can build on it.The problem
hatch run types:checkfailed immediately with 227 errors, sojust typecheckin #180 would have been broken out of the box. Two independent causes:typesenvironment installed only mypy, pydantic and numpy — nozarr,pytest,xarrayordask. Withignore_missing_imports = truethose all becameAny, producing ~90 phantom errors (untyped-decoratoron every pytest mark, staleunused-ignorecomments).mirrors-mypypre-commit hook, pinned to v1.15.0 and scoped tofiles: src. The hatch env resolved a different mypy and checkedteststoo, so the two could never agree.Provisioning the environment correctly dropped this to 134 errors and made
src/clean — the 19 apparent source errors were also artifacts of the missingzarrinstall.Single entry point
hatch run types:checkis now the only type-check command, run locally and by a newtypecheckCI job:[tool.hatch.envs.types]gainsdependency-groups = ["test"]and pinsmypy==2.3.0, so the versions mypy checks against come from the same place the tests get them and cannot drift.mypypre-commit hook is removed.sp-repo-review'sPC140hard-requires that hook, so it joins the existing[tool.repo-review] ignorelist.Trade-off worth naming: there is no longer mypy feedback at
git committime; the CI job is what catches it.Real bugs found
from_zarroverloads declareddepthwithout a default in bothv2andexperimental.v2, so the documented one-argument callfrom_zarr(group)did not type-check — for downstream users too, not just this repo.v3was already correct.experimental.v2.GroupSpec.from_flatwas narrower thanto_flat:to_flat()returnsBaseGroupSpecvalues butfrom_flat()accepted onlyGroupSpec, soGroupSpec.from_flat(group.to_flat())did not type-check. Widened to match the module-levelfrom_flat, which already accepted it.None == None:spec.filtersis a tuple whileauto_filters()returns the raw list, so any non-empty case would have failed. Now normalised before comparing.The remainder is mechanical: ~40 stale
type: ignorecomments removed, missing annotations added, andisinstancenarrowing in place of# type: ignore[attr-defined]on.memberschains — which makes those assertions stronger rather than merely quieter.Verification
hatch run types:check(src + tests)pre-commit run --all-filesOnly
srcchange with runtime effect is none — the two source fixes are annotation-level.tests/conftest.pydoes change at runtime (Structured(fields=...)now takes a tuple), which is why all three zarr versions were exercised.🤖 Generated with Claude Code