ci(mypy): add mypy check and adjust code for types #439
Conversation
d33bs
force-pushed
the
add-mypy-checks
branch
3 times, most recently
from
d517035
to
77d1bb4
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #439 +/- ##
=======================================
Coverage 94.72% 94.73%
=======================================
Files 56 57 +1
Lines 3147 3151 +4
=======================================
+ Hits 2981 2985 +4
Misses 166 166
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
d33bs
force-pushed
the
add-mypy-checks
branch
from
77d1bb4
to
7f6ea31
Compare
d33bs
force-pushed
the
add-mypy-checks
branch
from
7f6ea31
to
8478dbd
Compare
| if output_type is None: | ||
| output_type = "csv" |
There was a problem hiding this comment.
This feels dangerous - perhaps we could throw a warning that None is not supported?
There was a problem hiding this comment.
This is standard way of checking if a default none argument has been passed.
There was a problem hiding this comment.
ah, I should clarify what I mean by "dangerous" - if a user explicitly sets output_type=None and expects certain behavior (e.g., outputting to variable and not writing), they may be surprised that we overwrite it to output_type="csv". Perhaps the solution isn't to throw a warning (maybe add to docstring)? Or, we could elect to do nothing if this is standard expected behavior.
There was a problem hiding this comment.
Thanks for these comments! I had to make this change in order to integrate aggregate with output properly given its default parameter types. I recognize we could change the defaults one direction or the other (with aggregate or output) but I didn't want to move too much outside of the scope of the changes here in order to get a first pass with mypy. I wasn't sure about rationalizing a default value for aggregate's output_type if it wasn't going to use output. At the same time, I didn't want to change too much of output's default capabilities due to how it integrates with many other functions (could get complex for the scope of this PR).
My feeling here was that if output was used at all that we'd expect to be "outputting" something, and that if None were passed to it for output_type (especially for nested function calls) that it should revert to the default string value for the parameter in output.
| @@ -56,6 +56,20 @@ def test_output_default(): | |||
| result, DATA_DF, check_names=False, check_exact=False, atol=1e-3 | |||
| ) | |||
|
|
|||
| # test with an output_type of None | |||
|
Thanks @gwaybio and @kenibrewer for the reviews.
I'm not sure, but I wonder if this has to do with the low amount of type hinting alongside compatibility with inferred types. Could be useful to make a more focused effort towards adding type hints throughout.
When I looked here I noticed we no longer needed the |
Description
This PR adds a check through GitHub Actions for mypy in order to make sure Python types align with certain standards within pycytominer. I've also made some adjustments to the code in order to pass mypy checks.
Closes #437
What is the nature of your change?
Checklist
Please ensure that all boxes are checked before indicating that a pull request is ready for review.
📚 Documentation preview 📚: https://pycytominer--439.org.readthedocs.build/en/439/