-
Notifications
You must be signed in to change notification settings - Fork 35
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
ci(mypy): add mypy check and adjust code for types #439
base: main
Are you sure you want to change the base?
Conversation
d517035
to
77d1bb4
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #439 +/- ##
==========================================
+ Coverage 94.72% 94.73% +0.01%
==========================================
Files 56 57 +1
Lines 3147 3155 +8
==========================================
+ Hits 2981 2989 +8
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. |
77d1bb4
to
7f6ea31
Compare
7f6ea31
to
8478dbd
Compare
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.
LGTM, but I trust Ken more to give final approval
if output_type is None: | ||
output_type = "csv" |
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.
This feels dangerous - perhaps we could throw a warning that None is not supported?
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.
This is standard way of checking if a default none argument has been passed.
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
.
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.
This is a surprisingly low number of changes for enabling mypy
in a repo. This might be because of the ignore_missing_imports
and allow_redefinitions
config options. What was the change burden like for turning those options off?
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
|
This is now ready for another look when there's a moment @gwaybio and @kenibrewer . I've added more changes to include type stubs where reasonable. Many packages used by Generally I found that it'd be healthy to explore alternative type checkers in the future, especially |
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.
LGTM - let's wait on merging this until we merge #448 (after we merge 448, I am guessing you will need to resolve conflicts and rerun)
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.
LGTM too! Thanks for looking into the extra imports
Ready to merge @d33bs ! |
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/