-
Notifications
You must be signed in to change notification settings - Fork 586
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
Upgrade htsjdk to v3.0.1 and picard to 2.27.5 #8025
Conversation
Github actions tests reported job failures from actions build 3084888525
|
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## master #8025 +/- ##
===============================================
- Coverage 86.643% 86.642% -0.001%
+ Complexity 38929 38927 -2
===============================================
Files 2334 2334
Lines 182616 182612 -4
Branches 20049 20048 -1
===============================================
- Hits 158224 158219 -5
+ Misses 17357 17356 -1
- Partials 7035 7037 +2
|
Github actions tests reported job failures from actions build 3085525827
|
Github actions tests reported job failures from actions build 3229835669
|
fd3f0a5
to
bdcb3c5
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.
Looks good. Really only one question about the copy constructor confusion we just had.
private InverseAllele(final Allele allele, boolean isReference) { | ||
super(allele, false); |
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 shudder to imagine where else we have made this mistake...
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.
🤷
public AlleleAndContext(final String contig, final int loc, final Allele allele, final Allele refAllele) { | ||
super(allele, false); | ||
super(allele.getBases(), allele.isReference()); |
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.
can we just get rid of all the old calls to this copy constructor.
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.
There are no other calls to it in our codebase, we can deprecate it and replace it with a more appropriately named method.
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.
can you also maybe depricate it because its confusing as hell and a trap?
This upgrades htsjdk to v3.0.0 which was attempted in #7867 and then reverted in #7960 in order to unblock the jukebox merge.