Fix/ignore typing errors and add mypy to ci #38
Conversation
Codecov Report
@@ Coverage Diff @@
## main #38 +/- ##
==========================================
+ Coverage 91.74% 93.73% +1.99%
==========================================
Files 6 6
Lines 339 351 +12
==========================================
+ Hits 311 329 +18
+ Misses 28 22 -6
Continue to review full report at Codecov.
|
| self.attrs = attrs | ||
| self.array_type = array_type | ||
| self.checks = checks | ||
| # see https://github.com/python/mypy/issues/3004 |
There was a problem hiding this comment.
mypy seems to fail when it is applied to a property whose setter has a different return type than the parameter type of the corresponding getter. So I had to ignore these lines.
There was a problem hiding this comment.
@paiforsyth - thank you for the contribution. This all looks great. Two small comments but otherwise all seems good.
xarray_schema/dataset.py
Outdated
| @@ -27,7 +28,7 @@ def __init__( | |||
|
|
|||
| self.data_vars = data_vars | |||
| self.coords = coords | |||
| self.attrs = attrs | |||
| self.attrs = AttrSchema(attrs) | |||
There was a problem hiding this comment.
what's the reason for needing to do this here, as opposed to in the setter?
There was a problem hiding this comment.
I've fixed this to use a setter
There was a problem hiding this comment.
By the way, have you considered using dataclasses https://docs.python.org/3/library/dataclasses.html or attrs classes https://www.attrs.org/en/stable/ to represent schemas? They seem a natural fit.
There was a problem hiding this comment.
100% agree that we could transition to datacalsses. I may have even tried this in an earlier version. If memory serves, I ran into a problem with inheritance and decided to roll my own for the purposes of the MVP.
There was a problem hiding this comment.
If you'd like, we can discuss this idea in separate issue.
|
closing and reopening to re-trigger codecov |
for more information, see https://pre-commit.ci
|
I had to mark tasks.py as ignored by coverage to pass the coverage check |
|
Thanks @paiforsyth. I'm going to merge this now. Cheers! |
Thanks for this package! It looks like it could very useful!
I thought I would contribute by adding type checking.
I tried to add a mypy pre-commit hook, but this caused pre-commit CI to fail. I think this may be because the pre-commit CI is not able to install the correct requirements.