Add some more type signatures #227
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #227 +/- ##
========================================
Coverage 94.82% 94.82%
========================================
Files 32 32
Lines 5678 5679 +1
========================================
+ Hits 5384 5385 +1
Misses 294 294 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| @@ -33,14 +33,14 @@ def patch_version_lines(lines, build_number): | |||
| yield line | |||
|
|
|||
|
|
|||
| def patch_file(fname, build_number): | |||
There was a problem hiding this comment.
this is some CI only script, not sure we need types here.
There was a problem hiding this comment.
I don't think it costs much to have the types that are easy though, and having them enables at least some checking from MyPy.
There was a problem hiding this comment.
what's the point of return only type annotation on a file only used on CI? You are changing script that has NO types at all, to a script with VERY PARTIAL types, to me this is worse than the prior state. Please revert this part.
odc/geo/roi.py
Outdated
| @@ -77,7 +77,7 @@ def chunks(self) -> Chunks2d: ... | |||
|
|
|||
| def locate(self, pix: SomeIndex2d) -> Tuple[int, int]: ... | |||
|
|
|||
| def __dask_tokenize__(self): ... | |||
| def __dask_tokenize__(self) -> None: ... | |||
There was a problem hiding this comment.
pretty sure this function is supposed to return something
There was a problem hiding this comment.
https://github.com/dask/dask/blob/3a810721c0d6bba760cbf472f9ae907e809de92c/dask/typing.py#L215-L218
@abc.abstractmethod
def __dask_tokenize__(self) -> Hashable:
"""Value that must fully represent the object."""
raise NotImplementedError("Inheriting class must implement this method.")There was a problem hiding this comment.
I've updated the return type, but I would have expected an @override on the method so I think there's something more that should be fixed in this area. That is a separate issue though, I have no idea how these things are supposed to be stringed together.
There was a problem hiding this comment.
Does @override even makes sense on a Protocol type? It's really just formalizing duck-typing expected by your code.
pjonsson
force-pushed
the
more-types
branch
2 times, most recently
from
d36b9c7 to
0486430
Compare
No description provided.