Deprecate --allowMissingData, add --errorIfMissingData and print NA for null object in VariantsToTable #3190
Conversation
|
@vdauwera @lbergelson Please review. |
|
Ping @vdauwera @lbergelson |
There was a problem hiding this comment.
Doc/microcopy looks good, just a couple of typos to fix.
I'll leave the functional/code review to the esteemed @lbergelson
| @@ -74,7 +75,7 @@ | |||
| * | |||
| * <h3>Caveats</h3> | |||
| * <ul> | |||
| * <li>Some annotations cannot be applied to all variant sites, so VCFs typically contain records where some annotation values are missing. By default this tool throws an error if you request export of an annotation for which not all records have values. You can override this behavior by setting `--allowMissingData` in the command line. As a result, the tool will emit the special value NA for the missing annotations in those records.</li> | |||
| * <li>Some annotations cannot be applied to all variant sites, so VCFs typically contain records where some annotation values are missing. By default this tool the tool will emit the special value NA for the missing annotations if you request export of an annotation for which not all records have values. You can override this behavior by setting --errorIfMissingData in the command line. As a result, the tool will throw an error if a record is missing a value.</li> | |||
There was a problem hiding this comment.
"this tool the tool" -> fix
There was a problem hiding this comment.
Wow, I didn't realize I stutter when I type. 😉
Changed "this tool the tool" -> "this tool".
|
|
||
| /** | ||
| * By default, this tool will write out NA values indicating missing data when it encounters a field without a value in a record. | ||
| * If this flag is added to the command, the tool will instead exit with an error if missing data is encountered.. |
There was a problem hiding this comment.
Removed extra period.
| @Argument(fullName="allowMissingData", shortName="AMD", doc="If provided, we will not require every record to contain every field", optional=true) | ||
| @Hidden | ||
| @Deprecated | ||
| @Argument(fullName="allowMissingData", shortName="AMD", doc="This argument is no longer used, refer to the documentation for --errorIfMissingData", optional=true) |
There was a problem hiding this comment.
It seems like we should either delete this argument so it errors if someone uses it, or have it check and throw a reasonable exception if someone uses it. Hiding and deprecating it just means that it will silently do the wrong thing if someone tries to use it.
There was a problem hiding this comment.
The reasoning is that if a someone was using it, they still have the functionality. But, new users would not see this option.
@vdauwera Please weigh in.
There was a problem hiding this comment.
It's not to keep the functionality, it's that if you use the argument in a command you get a nice message saying "don't use this, use that one instead" rather than just an error about the argument not being found
There was a problem hiding this comment.
It's not to keep the functionality, it's that if you use the argument in a command you get a nice message saying "don't use this, use that one instead" rather than just an error about the argument not being found
There was a problem hiding this comment.
Although now that I think about it, in GATK4 we could go for a clean slate and just get rid of all the old GATK3 arguments since the upcoming refactoring is going to break all arguments anyway (we're still doing that, right?)
There was a problem hiding this comment.
@vdauwera Thanks for correcting me. I forgot that I removed all of the wiring to make this argument functional and the purpose of the @Deprecated annotation ...
There was a problem hiding this comment.
@vdauwera I don't think you'll get a nice message the way it is right now. I think it will just silently continue. You'd have to decide to look at the documentation, while enabling --showHidden in order to see the message. ( I don't think barclay understands automatic warning on deprecated arguments... ) If we want to show a message for a while as a transitionary step we'd have to add one manually. I'd vote for just killing it though.
There was a problem hiding this comment.
I removed the deprecated allowMissingData argument. @lbergelson please confirm you are you are still OK with that.
|
When you merge, be sure to update the commit comment and message to reflect the fact that it's been replaced instead of deprecated and that the default behavior is the opposite of what it used to be. |
ronlevine
force-pushed
the
rhl_variantstotable_npe_3188
branch
from
9011e2a
to
89c0171
Compare
…default behavior as previously) and print NA for null object in VariantsToTable
ronlevine
force-pushed
the
rhl_variantstotable_npe_3188
branch
from
89c0171
to
71a9b07
Compare
|
Change commit message to |
Codecov Report
@@ Coverage Diff @@
## master #3190 +/- ##
===============================================
- Coverage 80.498% 80.492% -0.006%
- Complexity 17509 17511 +2
===============================================
Files 1173 1173
Lines 63368 63375 +7
Branches 9876 9877 +1
===============================================
+ Hits 51010 51012 +2
- Misses 8409 8413 +4
- Partials 3949 3950 +1
|
Implements #3188