Skip to content

Update pre-commit hooks + mypy fixes #3133

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 1 commit into
base: main
Choose a base branch
from

Conversation

dstansby
Copy link
Contributor

Replaces #3109.

@github-actions github-actions bot added the needs release notes Automatically applied to PRs which haven't added release notes label Jun 13, 2025
@dstansby dstansby removed the needs release notes Automatically applied to PRs which haven't added release notes label Jun 13, 2025
@github-actions github-actions bot added the needs release notes Automatically applied to PRs which haven't added release notes label Jun 17, 2025
@dstansby
Copy link
Contributor Author

@d-v-b any ideas what's going on with the typing error here? I can't wrap my head around why it's erroring. Perhaps we should just stick an # type: ignore comment on the failing line?

@dstansby dstansby removed the needs release notes Automatically applied to PRs which haven't added release notes label Jun 20, 2025
@dstansby dstansby mentioned this pull request Jun 20, 2025
6 tasks
@DimitriPapadopoulos
Copy link
Contributor

DimitriPapadopoulos commented Jun 20, 2025

I wonder whether this would fix the mypy error:

-    if dtype_category == "variable-length-string":
-        zdtype = VariableLengthUTF8()
-    else:
-        zdtype = Int8()
+    if dtype_category == "variable-length-string":
+        zdtype = VariableLengthUTF8
+    else:
+        zdtype = Int8

@github-actions github-actions bot added the needs release notes Automatically applied to PRs which haven't added release notes label Jun 20, 2025
@dstansby
Copy link
Contributor Author

The issue with the dtype mypy error was the variable length string type is not actually a valid ZarrDtype. Since there are existing #type: ignore comments to acocunt for this, I added another one here.

@d-v-b
Copy link
Contributor

d-v-b commented Jun 20, 2025

@d-v-b any ideas what's going on with the typing error here? I can't wrap my head around why it's erroring. Perhaps we should just stick an # type: ignore comment on the failing line?

which specific typing error? a basic challenge with VariableLengthUTF8 is that it's conditionally defined: if numpy < 2 is installed, we define 1 version of the class (using the numpy object dtype) and if numpy > 2 is installed, we define a different version of the class (using np.dtypes.StringDType). That can be why mypy singles this type out as unknown

@dstansby
Copy link
Contributor Author

@d-v-b
Copy link
Contributor

d-v-b commented Jun 20, 2025

This line here: https://github.com/zarr-developers/zarr-python/pull/3133/files#diff-da24b355349b53e4c8047034767d008e4c2ce9ea6c8933a3acd50d0828f00528R334 - if you remove the typing ignores, it gives an error.

yeah, mypy is confused there, so the type ignore is correct. This will get simpler in a few months when we drop numpy < 2.

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