Add type checking (mypy) #1244
Merged
Add type checking (mypy) #1244
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## master #1244 +/- ##
==========================================
- Coverage 68.96% 68.96% -0.01%
==========================================
Files 64 64
Lines 6944 6946 +2
Branches 1693 1696 +3
==========================================
+ Hits 4789 4790 +1
Misses 1851 1851
- Partials 304 305 +1
☔ View full report in Codecov by Sentry. |
victorlin
force-pushed
the
victorlin/add-type-checking
branch
6 times, most recently
from
0e7a2b3
to
04e7027
Compare
|
@corneliusroemer you read my mind! I was going to include pyright here but I think it's better to split that out into a separate PR. |
joverlee521
reviewed
Jun 29, 2023
victorlin
force-pushed
the
victorlin/add-type-checking
branch
from
bf4ff71
to
8c19b75
Compare
joverlee521
approved these changes
Jun 30, 2023
The codebase has sprinkles of typing here and there. Previously, they were not checked in any way and purely for extra context to readers of the code. Now, they are checked and enforced through mypy. There are several mypy errors when run against the code as-is. The following commits will address those errors. Setup was largely copied from existing mypy usage in Nextstrain CLI, including the way mypy is run via pytest.
For each external package that was missing types, add a separate type packages if available. Otherwise, ignore the absence of types.
The violations are surrounded by existing context that justify exceptions.
Minor non-functional changes to appease the type-checker. To me, the changes also better describe the code's purpose.
This shows the effects of an existing bug, to be fixed in the following commit.
victorlin
force-pushed
the
victorlin/add-type-checking
branch
from
8c19b75
to
2a74460
Compare
Previously, if --fix-dates was not specified, it would behave as if --fix-dates='monthfirst' were used. This change was inadvertently left out of e82d314. Previous code ran without errors because `fix_dates` is the name of a function. The bug was noticed by a Mypy warning hinting that `fix_dates` will always be truthy.
The type of the delimiter parameter to csv.Sniffer.sniff is str, so that's what should be accepted here. Also check to ensure that delimiters are single characters.
victorlin
force-pushed
the
victorlin/add-type-checking
branch
from
ebf4c97
to
a5c173a
Compare
1 task
3 tasks
tsibley
reviewed
Jul 5, 2023
| @@ -290,7 +290,7 @@ def available_cpu_cores(fallback: int = 1) -> int: | |||
| # naively use all 24, we'd end up with two threads across the 12 cores. | |||
| # This would degrade performance rather than improve it! | |||
| return len(os.sched_getaffinity(0)) | |||
| except: | |||
| else: | |||
There was a problem hiding this comment.
There's a semantic change here in the edge case, because os.sched_getaffinity(0) can fail and raise a Python exception. When it does, Augur will be broken with the change to if/else instead of falling back to os.cpu_count() with the original try/except.
I think this change should be reverted and mypy appeased another way.
|
Hooray for types! \o/ |
2 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description of proposed changes
These changes enforce the types that are sprinkled in the codebase here and there, and ensure future types are checked.
Related issue(s)
--fix-datesis not specified #1247Testing
What steps should be taken to test the changes you've proposed?
If you added or changed behavior in the codebase, did you update the tests, or do you need help with this?
Checklist