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

refactored table writing in BasicSomaticValidator #6136

Merged
merged 2 commits into from
Sep 6, 2019

Conversation

davidbenjamin
Copy link
Contributor

@samuelklee This is for you regarding #3884.

Copy link
Contributor

@samuelklee samuelklee left a comment

Choose a reason for hiding this comment

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

Thanks so much for cleaning this up so quickly, @davidbenjamin! A few minor quibbles, OK to merge after.

@@ -2,6 +2,20 @@

import htsjdk.samtools.util.Locatable;
import htsjdk.variant.variantcontext.Allele;
import org.broadinstitute.hellbender.exceptions.UserException;
import org.broadinstitute.hellbender.tools.walkers.contamination.ContaminationRecord;
Copy link
Contributor

Choose a reason for hiding this comment

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

Optimize imports throughout.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@@ -104,4 +118,112 @@ public String getFilters() {
public long getNumAltSupportingReadsInNormal() {
return numAltSupportingReadsInNormal;
}

//----- The following two public static methods read and write files
public static void writeToFile(final List<BasicValidationResult> records, final File outputTable) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think just write and read as method names are fine, but up to you. Might also consider just file for the parameter names, or perhaps outputFile and inputFile (the distinction between the current outputTable and tableFile is not clear).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done


//----- The following two public static methods read and write files
public static void writeToFile(final List<BasicValidationResult> records, final File outputTable) {
try ( final BasicValidationResult.BasicValidationResultTableWriter writer = new BasicValidationResult.BasicValidationResultTableWriter(IOUtils.fileToPath(outputTable)) ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Some inconsistencies in white space here and in the try statement below.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

try ( final BasicValidationResult.BasicValidationResultTableWriter writer = new BasicValidationResult.BasicValidationResultTableWriter(IOUtils.fileToPath(outputTable)) ) {
writer.writeHeaderIfApplies();
writer.writeAllRecords(records);
} catch (IOException e){
Copy link
Contributor

Choose a reason for hiding this comment

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

Make the exception final here and below.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

}

//-------- The following methods are boilerplate for reading and writing tables
private static class BasicValidationResultTableWriter extends TableWriter<BasicValidationResult> {
Copy link
Contributor

Choose a reason for hiding this comment

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

Reconsider the access modifiers of the below classes (and their constructors) and enum (and its COLUMNS field)---some inconsistencies here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@davidbenjamin davidbenjamin merged commit 3b4278d into master Sep 6, 2019
@davidbenjamin davidbenjamin deleted the db_annotated_interval branch September 6, 2019 16:37
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