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
Add type hints to zarr.create #1536
Conversation
I like this! I left some comments. Having started one of these "add types to zarr" endeavors before, I understand that it can feel unbounded, so I respect your restraint in only adding types where you did :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
any objections to merging this? |
I'm kicking off the gha workflows, assuming those pass I'm going to merge this unless there are any objections. |
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #1536 +/- ##
==========================================
- Coverage 99.99% 99.99% -0.01%
==========================================
Files 37 38 +1
Lines 14738 14599 -139
==========================================
- Hits 14737 14598 -139
Misses 1 1
|
I know that @d-v-b and I tend to be fans, but this feels like something that others may want to take a look. Anyone want us to hold merging, @zarr-developers/python-core-devs? |
Since this doesnt affect runtime at all, I think its safe to merge this. |
It is adding a new public module though. I would give it until next week following https://github.com/zarr-developers/governance/blob/main/GOVERNANCE.md#decision-making-process |
"""Base class for synchronizers.""" | ||
|
||
def __getitem__(self, item): | ||
... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thoughts welcome on how to make codecov happy with these lines.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I couldn't find anything obvious in https://docs.codecov.com/docs/ignoring-paths, seems like it only has per-file granularity. Since we know the decrease in coverage isn't meaningful, I'd be fine ignoring it here. In the future maybe we put protocols in types.py
(either a module-local one, or a zarr-wide one) and tell codecov to ignore it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've updated the code coverage config to ignore lines that are "...".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good idea!
thanks @dstansby! and please feel free to dive in to the v3 effort! |
* Add type hints to zarr.create * Use protocol for MetaArray * Use protocol for Synchronizer * Fix Path typing * Add release note * Fix dim separator typing * Ignore ... in coverage reporting * Fix chunk typing --------- Co-authored-by: Davis Bennett <davis.v.bennett@gmail.com>
This PR adds type hints to
zarr.creation.create()
.I couldn't find any discussion of adding type hints to
zarr
in the issues or discussion forums, so not sure if this is welcome - I would definitely personally find it helpful to have type hints throughout the codebase, and would be happy to chip away at adding them if they are. But definitely understand if it would be useful to have a wider discussion separate to this PR about adding type hinting tozarr
before going any further.TODO: