Skip to content
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

Enable checking of untyped definitions #1612

Closed
wants to merge 5 commits into from

Conversation

dstansby
Copy link
Contributor

@dstansby dstansby commented Dec 18, 2023

This enables some more mypy checks. The first two didn't need any fixes. check_untyped_defs requires quite a lot of fixes. I've made some of them, and added a list of modules that need a lot of fixes to ignore for now. The idea here is that at least in any other newly added modules untyped definitions will be checked.

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/tutorial.rst
  • Changes documented in docs/release.rst
  • GitHub Actions have all passed
  • Test coverage is 100% (Codecov passes)

@@ -833,7 +834,7 @@ def __getstate__(self):

def __setstate__(self, state):
root, cls = state
self.__init__(root=root, cls=cls)
self.__init__(root=root, cls=cls) # type: ignore[misc]
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These igore statements are all for the following warning. I'm not sure if there's a good way to fix instead of ignoring the warning?

Accessing "init" on an instance is unsound, since instance.init could be from an incompatible subclass

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pretty sure the right thing here is to define__reduce__, which I started over in #1089. It's stalled, but very simple to pick up I think

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 - do you want #1089 to be finished before merging this PR, or would it be okay to merge this with the # type: ignore comments and then get rid of them when #1089 is done?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think blinding mypy to our sins with #type: ignore is fine for now

Copy link

codecov bot commented Dec 18, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.99%. Comparing base (6105ef2) to head (80293c5).

❗ Current head 80293c5 differs from pull request most recent head cd5ed41. Consider uploading reports for the commit cd5ed41 to get more accurate results

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #1612   +/-   ##
=======================================
  Coverage   99.98%   99.99%           
=======================================
  Files          38       38           
  Lines       14640    14575   -65     
=======================================
- Hits        14638    14574   -64     
+ Misses          2        1    -1     
Files Coverage Δ
zarr/_storage/v3.py 100.00% <100.00%> (ø)
zarr/attrs.py 100.00% <100.00%> (ø)
zarr/core.py 100.00% <100.00%> (ø)
zarr/hierarchy.py 100.00% <100.00%> (+0.20%) ⬆️
zarr/indexing.py 100.00% <100.00%> (ø)
zarr/n5.py 100.00% <100.00%> (ø)
zarr/storage.py 100.00% <100.00%> (ø)
zarr/sync.py 100.00% <100.00%> (ø)
zarr/util.py 100.00% <ø> (ø)

... and 2 files with indirect coverage changes

@dstansby
Copy link
Contributor Author

Anything more to do here before this can be merged?

@MSanKeys963 MSanKeys963 requested a review from d-v-b April 9, 2024 23:30
@dstansby
Copy link
Contributor Author

I'll close this since it's gone in to the v3 branch

@dstansby dstansby closed this Apr 14, 2024
@dstansby dstansby deleted the check-untyped-defs branch April 14, 2024 08:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants