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.

Sign up for GitHub

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

Jump to bottom

Add a method to conditionally apply a Consumer on a nullable input #1419

Merged
merged 3 commits into from
Sep 23, 2019
Merged

Add a method to conditionally apply a Consumer on a nullable input #1419

merged 3 commits into from
Sep 23, 2019

Conversation

yfarjoun
Copy link
Contributor

Description

Please explain the changes you made here.
Explain the motivation for making this change. What existing problem does the pull request solve?

Things to think about before submitting:

  • Make sure your changes compile and new tests pass locally.
  • Add new tests or update existing ones:
    • A bug fix should include a test that previously would have failed and passes now.
    • New features should come with new tests that exercise and validate the new functionality.
  • Extended the README / documentation, if necessary
  • Check your code style.
  • Write a clear commit title and message
    • The commit message should describe what changed and is targeted at htsjdk developers
    • Breaking changes should be mentioned in the commit message.

@codecov-io
Copy link

codecov-io commented Sep 20, 2019 •
edited
Loading

Codecov Report

Merging #1419 into master will increase coverage by 0.318%.
The diff coverage is 83.333%.

@@               Coverage Diff               @@
##              master     #1419       +/-   ##
===============================================
+ Coverage     68.127%   68.445%   +0.318%     
- Complexity      8381      8673      +292     
===============================================
  Files            573       573               
  Lines          33985     34873      +888     
  Branches        5668      5930      +262     
===============================================
+ Hits           23153     23869      +716     
- Misses          8643      8783      +140     
- Partials        2189      2221       +32
Impacted Files Coverage Δ Complexity Δ
src/main/java/htsjdk/samtools/util/IOUtil.java 61.689% <ø> (+3.276%) 196 <0> (+76) ⬆️
src/main/java/htsjdk/samtools/util/CodeUtil.java 71.429% <83.333%> (+4.762%) 4 <4> (+2) ⬆️
...htsjdk/samtools/util/nio/DeleteOnExitPathHook.java 86.667% <0%> (-3.81%) 5% <0%> (+1%)
.../htsjdk/samtools/util/AbstractRecordAndOffset.java 81.081% <0%> (-2.919%) 15% <0%> (+6%)
...in/java/htsjdk/samtools/util/SamLocusIterator.java 90.588% <0%> (-1.193%) 23% <0%> (ø)
.../variant/variantcontext/VariantContextBuilder.java 81.992% <0%> (-0.949%) 75% <0%> (+35%)
.../htsjdk/variant/variantcontext/VariantContext.java 79.668% <0%> (+1.664%) 345% <0%> (+108%) ⬆️
...c/main/java/htsjdk/samtools/liftover/LiftOver.java 85.167% <0%> (+2.31%) 44% <0%> (+13%) ⬆️
.../samtools/reference/AbstractFastaSequenceFile.java 60% <0%> (+2.5%) 18% <0%> (+6%) ⬆️
src/main/java/htsjdk/variant/vcf/VCFEncoder.java 88.652% <0%> (+2.794%) 96% <0%> (+28%) ⬆️
... and 3 more

lbergelson
lbergelson requested changes Sep 20, 2019
View reviewed changes
Copy link
Member

@lbergelson lbergelson left a comment

Choose a reason for hiding this comment

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

@yfarjoun This seems like a reasonable method to add. Two comments and then 👍

src/main/java/htsjdk/samtools/util/CodeUtil.java
}

public static <T> void applyIfNotNull(final T input, Consumer<T> action) {
Copy link
Member

Choose a reason for hiding this comment

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

javadoc

src/test/java/htsjdk/samtools/util/IOUtilTest.java Outdated
@@ -114,6 +114,10 @@ public void testFileReadingAndWriting() throws IOException {
String randomizedTestString = TEST_STRING + System.currentTimeMillis();
for (String ext : TEST_FILE_EXTENSIONS) {
File f = File.createTempFile(TEST_FILE_PREFIX, ext);
IOUtil.assertFileIsReadable(f);

CodeUtil.applyIfNotNull(f,IOUtil::assertFileIsReadable);
Copy link
Member

Choose a reason for hiding this comment

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

This is a strange test for this method... Why not give it its own test case in a new CodeUtilUnitTest class?

lbergelson
lbergelson approved these changes Sep 23, 2019
View reviewed changes
Copy link
Member

@lbergelson lbergelson left a comment

Choose a reason for hiding this comment

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

👍

@yfarjoun yfarjoun merged commit e357c42 into master Sep 23, 2019
@yfarjoun yfarjoun deleted the yf_add_conditionallyCheckReadable branch September 23, 2019 18:51
@yfarjoun yfarjoun restored the yf_add_conditionallyCheckReadable branch August 11, 2020 16:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lbergelson lbergelson lbergelson approved these changes

@pshapiro4broad pshapiro4broad Awaiting requested review from pshapiro4broad

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@yfarjoun @codecov-io @lbergelson