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

Align the Funcotator checkIfAlreadyAnnotated test with the Funcotator engine code. #7555

Merged
merged 1 commit into from
Nov 22, 2021

Conversation

cmnbroad
Copy link
Collaborator

@cmnbroad cmnbroad commented Nov 9, 2021

@jonn-smith Quick one when you get a chance - this fixes some things I noticed on my GATK branch when testing my new htsjdk VCFHeader code. The Funcotator.checkIfAlreadyAnnotated code was checking for a header line that was never generated by Funcotator AFAICT, and this also ties together the Funcotator engine and test code to use the same constants. More could probably be done here but it would require a bigger refactoring.

droazen
droazen previously requested changes Nov 9, 2021
public static final String VCF_HEADER_ALREADY_ANNOTATED_1 = "Funcotator";

/** Name of the second header line key which indicates that a file has already been annotated. */
public static final String VCF_HEADER_ALREADY_ANNOTATED_2 = "Funcotator Version";
Copy link
Collaborator

Choose a reason for hiding this comment

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

I believe there may have been a good reason for checking both of these -- see the original PR for context (#7349). @jonn-smith can confirm one way or another when he returns.

Copy link
Collaborator Author

@cmnbroad cmnbroad Nov 9, 2021

Choose a reason for hiding this comment

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

Ok, but I can find neither a file that contains the other line, nor code that sets it. Maybe its a legacy thing, but if not I just need to know where the code is that creates the first header line (plain "Funcotator").

Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think that there is necessarily a good reason for this check. I think the updates are good.

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.

This looks like a good change to me. I don't think there should be any issues moving to this check (as compared with the previous implementation).

@cmnbroad cmnbroad dismissed droazen’s stale review November 22, 2021 18:58

Approved by Jonn.

@cmnbroad cmnbroad merged commit eeb9e1f into master Nov 22, 2021
@cmnbroad cmnbroad deleted the cn_funcotator_header branch November 22, 2021 18:58
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