Skip to content

Conversation

d-v-b
Copy link
Contributor

@d-v-b d-v-b commented Jul 14, 2025

mypy was flagging problems with some of the obstore type annotations. I don't know what change precipitated these new errors, but this PR silences them by adding type: ignore statements. Obviously fixing the type annotations would be even better, but several obvious things that I attempted did not work, so I think we need an obstore expert for a real fix (@kylebarron)

until then, this PR keeps our CI clean

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 Jul 14, 2025
@d-v-b
Copy link
Contributor Author

d-v-b commented Jul 14, 2025

I'm very confused now, because the type:ignore statements necessary to pass mypy locally fail our mypy CI job. I'm removing those type hints and re-running things.

@d-v-b
Copy link
Contributor Author

d-v-b commented Jul 14, 2025

test failures are unrelated.

@d-v-b d-v-b requested a review from a team July 14, 2025 19:22
@TomAugspurger
Copy link
Contributor

mypy locally fail our mypy CI job

You might need to update your version of obstore locally.

@d-v-b
Copy link
Contributor Author

d-v-b commented Jul 14, 2025

mypy locally fail our mypy CI job

You might need to update your version of obstore locally.

I was running mypy against obstore upstream, so maybe I needed to downgrade

@d-v-b d-v-b merged commit cea611b into zarr-developers:main Jul 14, 2025
27 of 28 checks passed
@d-v-b d-v-b deleted the obstore-type-hints branch July 14, 2025 19:26
@kylebarron
Copy link
Contributor

Indeed in Obstore we switched from list to Sequence because Sequence is covariant while list is invariant. While the physical type returned is indeed a list, typing it as a Sequence more faithfully represents the semantics. It was also necessary for the work with obspec

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