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

Clearly label the number of reads in the CountReads output #6449

Merged
merged 1 commit into from
Feb 21, 2020

Conversation

droazen
Copy link
Contributor

@droazen droazen commented Feb 10, 2020

No description provided.

@droazen
Copy link
Contributor Author

droazen commented Feb 10, 2020

@lbergelson A quick one for you

@@ -39,6 +39,7 @@ public void apply( final GATKRead read, final ReferenceContext referenceContext,

@Override
public Object onTraversalSuccess() {
logger.info("CountReads counted " + count + " total reads");
Copy link
Member

Choose a reason for hiding this comment

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

@droazen This should probably print a non-logger number directly to standard out as well shouldn't it? So that people can capture it programmatically/ incase logging is turned off.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This PR is just to label the value output by the tool, which was confusing users. If we want to support actual piping of the output, which is not something we typically do, we'd probably want an actual test case to prove that it works. This seems beyond the scope of this particular PR.

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.

@droazen One comment, this is definitely an improvement over not specifying anything, but maybe we want more?

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.

👍

@droazen droazen merged commit 736bdd5 into master Feb 21, 2020
@droazen droazen deleted the dr_label_countreads_output branch February 21, 2020 15:00
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.

2 participants