Skip to content
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

Added name of offending record to error message in SamPairUtil #1358

Merged
merged 1 commit into from
May 7, 2019

Conversation

yfarjoun
Copy link
Contributor

This PR was inspired by broadinstitute/picard#201 where the error doesn't allow the user to investigate the offending record. By adding the record name to the exception message, the user can investigate more easily and fix the input.

@yfarjoun
Copy link
Contributor Author

Apologies about the CRLF->LF change. please use "hide whitespaces" to view the actual code change.

@codecov-io
Copy link

codecov-io commented Apr 25, 2019

Codecov Report

Merging #1358 into master will increase coverage by 0.174%.
The diff coverage is 57.488%.

@@               Coverage Diff               @@
##              master     #1358       +/-   ##
===============================================
+ Coverage     67.849%   68.024%   +0.174%     
- Complexity      8283      8363       +80     
===============================================
  Files            563       563               
  Lines          33707     33994      +287     
  Branches        5657      5720       +63     
===============================================
+ Hits           22870     23124      +254     
- Misses          8659      8687       +28     
- Partials        2178      2183        +5
Impacted Files Coverage Δ Complexity Δ
src/main/java/htsjdk/samtools/SamPairUtil.java 57.488% <57.488%> (ø) 20 <20> (ø) ⬇️
...n/java/htsjdk/samtools/cram/structure/SliceIO.java 84.524% <0%> (+1.19%) 15% <0%> (+1%) ⬆️
...antcontext/writer/VariantContextWriterBuilder.java 85.597% <0%> (+2.794%) 110% <0%> (+50%) ⬆️
...java/htsjdk/samtools/cram/structure/Container.java 91.781% <0%> (+3.737%) 31% <0%> (+4%) ⬆️
...samtools/util/AsyncBlockCompressedInputStream.java 76% <0%> (+4%) 13% <0%> (+1%) ⬆️
...ain/java/htsjdk/samtools/cram/structure/Slice.java 72.174% <0%> (+4.581%) 67% <0%> (+24%) ⬆️
...va/htsjdk/samtools/cram/structure/ContainerIO.java 68.056% <0%> (+4.722%) 10% <0%> (ø) ⬇️

Copy link
Collaborator

@cmnbroad cmnbroad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Change itself seems fine to me.

// No need to advance if we have records remaining
if (!records.isEmpty()) return;

/*
Copy link
Contributor

@pshapiro4broad pshapiro4broad May 6, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Multi-line comments usually have a * character at the start of each line. Or, you could use // for each line of a multi-line comment.

Can you make another PR that doesn't show every line as being different? Nevermind, I see you updated from DOS->Unix.

@yfarjoun yfarjoun merged commit 7ce3636 into master May 7, 2019
@yfarjoun yfarjoun deleted the yf_improve_error_in_mate_info branch May 7, 2019 00:29
@yfarjoun yfarjoun restored the yf_improve_error_in_mate_info branch August 11, 2020 16:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants