-
Notifications
You must be signed in to change notification settings - Fork 2
Improvement/sdev 5193 mypy type fixes #81
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
base: develop
Are you sure you want to change the base?
Conversation
…stead of Iterable type for len property
… type matches description
…rom IprSmallMutationVariant gene vs IprFusionVariant gene1 and gene2
…bledict conflicts
Codecov Report❌ Patch coverage is
❌ Your patch status has failed because the patch coverage (67.34%) is below the target coverage (80.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## develop #81 +/- ##
===========================================
+ Coverage 83.10% 83.36% +0.26%
===========================================
Files 18 18
Lines 2438 2441 +3
===========================================
+ Hits 2026 2035 +9
+ Misses 412 406 -6
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
…mprovement/SDEV-5193_mypy_type_fixes
…mprovement/SDEV-5193_mypy_type_fixes
…b.com/bcgsc/pori_python into improvement/SDEV-5193_mypy_type_fixes
| variants: List[IprExprVariant], | ||
| show_progress: bool = False, | ||
| ) -> List[KbMatch]: | ||
| ) -> List[Hashabledict]: |
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.
Why these need to be hashable? We're loosing all the structure of KbMatch.
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.
Hashabledict is used for dropping duplicates from a list (convert to set).
The function seems to be used in exactly one place and immediately converted to a hashable type.
Ideally, I would have liked to just make KbMatch a hashable dict type, but I couldn't figure that out here. If someone can figure that out, I think that is the appropriate solution.
I was just trying to something that gave the appropriate mypy warnings and didn't create errors from any of the KbMatch duplicates.
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.
I think probably best to preserve KbMatch structure; will take a crack at making kbmatch a hashable dict type.
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.
I think probably best to preserve KbMatch structure; will take a crack at making kbmatch a hashable dict type.
Hi @elewis2, are you having any luck with this?
I think we will want to start with these changes regardless and add extra typing details as we can.
It's more important that the parts where structure is given is accurate than the fact that it is lost at some point. At least this way the mypy warnings are accurate.
The other way to keep the KbMatch structure would be to refactor to eliminate the Hashabledict all together. If I understand it was only used to drop duplicates. I think you could eliminate the Hashabledict by creating functions that can take lists of kbMatches and drop duplicates.
Changes to eliminate mypy errors and allow type checking