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

Replacing some uses of PrintWriter #5461

Merged
merged 2 commits into from
Nov 29, 2018

Conversation

lbergelson
Copy link
Member

  • PrintWriter doesn't throw exceptions on write failures and might therefore lead to unexpectedly trunacated data being produced without any notification
  • replacing it's use in MetricsUtils and MafOutputRenderer where it might cause problems
  • partial fix for Remove uses of PrintWriter #5458

* PrintWriter doesn't throw exceptions on write failures and might therefore lead to unexpectedly trunacated data being produced without any notification
* replacing it's use in MetricsUtils and MafOutputRenderer where it might cause problems
* partial fix for #5458
Copy link
Collaborator

@jonn-smith jonn-smith left a comment

Choose a reason for hiding this comment

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

Looks good, but you missed one of the print writer instances.

@@ -260,7 +261,7 @@ public MafOutputRenderer(final Path outputFilePath,

// Open the output object:
try {
printWriter = new PrintWriter(Files.newOutputStream(outputFilePath));
writer = new PrintWriter(Files.newOutputStream(outputFilePath));
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think you missed one. :P

Copy link
Collaborator

@jonn-smith jonn-smith left a comment

Choose a reason for hiding this comment

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

Looks good to me!

When tests pass feel free to merge.

@codecov-io
Copy link

codecov-io commented Nov 29, 2018

Codecov Report

Merging #5461 into master will decrease coverage by 0.005%.
The diff coverage is 83.673%.

@@              Coverage Diff               @@
##             master     #5461       +/-   ##
==============================================
- Coverage     86.99%   86.985%   -0.005%     
+ Complexity    31202     31200        -2     
==============================================
  Files          1909      1909               
  Lines        144184    144195       +11     
  Branches      15951     15951               
==============================================
+ Hits         125425    125428        +3     
- Misses        12991     12999        +8     
  Partials       5768      5768
Impacted Files Coverage Δ Complexity Δ
...roadinstitute/hellbender/metrics/MetricsUtils.java 57.143% <50%> (ø) 1 <1> (ø) ⬇️
.../tools/funcotator/mafOutput/MafOutputRenderer.java 93.651% <85.106%> (-1.233%) 80 <4> (ø)
...utils/smithwaterman/SmithWatermanIntelAligner.java 50% <0%> (-30%) 1% <0%> (-2%)
...ithwaterman/SmithWatermanIntelAlignerUnitTest.java 60% <0%> (ø) 2% <0%> (ø) ⬇️
...oadinstitute/hellbender/utils/gcs/BucketUtils.java 81.098% <0%> (+0.61%) 42% <0%> (ø) ⬇️

@lbergelson lbergelson merged commit d95ceec into master Nov 29, 2018
@lbergelson lbergelson deleted the lb_replace_some_uses_of_printwriter branch November 29, 2018 18:07
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.

3 participants