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

FilterSamReads has a bug and can't filter read by read name. #1055

Merged
merged 2 commits into from Jan 10, 2018

Conversation

yfarjoun
Copy link
Contributor

@yfarjoun yfarjoun commented Jan 7, 2018

  • fixed this bug
  • added positive and negative tests

fixes #1070

Description

Give your PR a concise yet descriptive title
Please explain the changes you made here.
Explain the motivation for making this change. What existing problem does the pull request solve?
Mention any issues fixed, addressed or otherwise related to this pull request, including issue numbers or hard links for issues in other repos.
You can delete these instructions once you have written your PR description.


Checklist (never delete this)

Never delete this, it is our record that procedure was followed. If you find that for whatever reason one of the checklist points doesn't apply to your PR, you can leave it unchecked but please add an explanation below.

Content

  • Added or modified tests to cover changes and any new functionality
  • Edited the README / documentation (if applicable)
  • All tests passing on Travis

Review

  • Final thumbs-up from reviewer
  • Rebase, squash and reword as applicable

For more detailed guidelines, see https://github.com/broadinstitute/picard/wiki/Guidelines-for-pull-requests

- fixed this bug
- added positive and negative tests
@coveralls
Copy link

coveralls commented Jan 7, 2018

Coverage Status

Coverage increased (+0.08%) to 77.731% when pulling f1cf54c on yf_filterSamReads_bugfix into 7accdcb on master.

Copy link
Contributor

@gbggrant gbggrant 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. A couple of minor suggestions.

}
}

final File reads = File.createTempFile(TEST_DIR.getAbsolutePath(),"reads");
Copy link
Contributor

Choose a reason for hiding this comment

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

space after comma

String.format("%s=%s", fileArgument, dummyFile),
};

// make sure results is successful
Copy link
Contributor

Choose a reason for hiding this comment

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

update comment 'make sure program invocation failed - inputs validation error'

@Test(dataProvider = "badArgumentCombinationsdata")
public void testBadArgumentCombinations(final FilterSamReads.Filter filter, final String fileArgument) throws IOException {

final File dummyFile = File.createTempFile(TEST_DIR.getAbsolutePath(),"dummy");
Copy link
Contributor

Choose a reason for hiding this comment

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

space after comma

program.READ_LIST_FILE = inputFile;
break;
default:
throw new IllegalArgumentException("not configured for filter==" + filter);
Copy link
Contributor

Choose a reason for hiding this comment

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

double '=' ?

"INPUT=testdata/picard/sam/aligned.sam",
String.format("OUTPUT=%s", temp.getAbsolutePath()),
String.format("FILTER=%s", filter.toString()),
String.format("%s=%s", fileArgument, dummyFile),
Copy link
Contributor

Choose a reason for hiding this comment

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

dummyFile.getAbsolutePath()

@gbggrant gbggrant self-requested a review January 10, 2018 16:18
Copy link
Contributor

@gbggrant gbggrant left a comment

Choose a reason for hiding this comment

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

👍

@coveralls
Copy link

coveralls commented Jan 10, 2018

Coverage Status

Coverage increased (+0.1%) to 77.766% when pulling fff40ef on yf_filterSamReads_bugfix into 7accdcb on master.

@yfarjoun yfarjoun merged commit ed43b59 into master Jan 10, 2018
@yfarjoun yfarjoun deleted the yf_filterSamReads_bugfix branch January 10, 2018 19:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

bug report FilterSamReads
3 participants