-
Notifications
You must be signed in to change notification settings - Fork 589
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
making gatk --version print the version #5537
Conversation
This is different than the output of the tool --version...
I can change this to be that version, or we can change barclay to do it differently and provide more info. It might be useful to print the library versions with both commands as well.. that's printed during the normal run...
@cmnbroad What do you think? |
System.out.println(String.format("%s v%s", getToolkitName(), getVersion())); | ||
return 0; | ||
} | ||
}.doWork(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be nice to find a solution that worked for downstream toolkits - I think this would only work if they repackaged GATK classes in their own jar ? I'm not sure I see an obvious solution that doesn't involve a lot more work though.
Either way, what do you think about adding a couple of methods to RuntimeUtils or somewhere that get the toolkit name and version number from the manifest, and then call those from here and from the corresponding CommandLineProgram methods ? It still wouldn't completely solve the downstream toolkit issue, but at least we wouldn't have to spin up a dummy CLP. Longer term I really want to fix the special argument handling and delegation in Barclay.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
BTW, by "from the manifest" I meant "they way they do now", ie., getClass().getPackage().getImplementationVersion().
This is great. For what it's worth, I would love for the Picard and HTSJDK versions to print also with |
ce58ba3
to
54cb2be
Compare
@cmnbroad I extracted a bunch of methods to RunTimeUtils and made it cleaner. I fixed a todo in CommandLineProgram that was related as well. It's hard to test these things because the tests sometimes but don't always the jar with a manifest so I didn't test most of the new methods. I can try to do something more clever if you think it's necessary though... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great when test pass - thx for doing all that.
Codecov Report
@@ Coverage Diff @@
## master #5537 +/- ##
===============================================
- Coverage 87.085% 87.075% -0.009%
- Complexity 31222 31227 +5
===============================================
Files 1915 1915
Lines 144079 144097 +18
Branches 15891 15899 +8
===============================================
+ Hits 125471 125473 +2
- Misses 12837 12841 +4
- Partials 5771 5783 +12
|
* extracting methods from CommandLineProgram -> RuntimeUtils for getting version and manifest attributes * fixing a todo in CommandLineProgram * opportunistically changing an instance of map.entryset().stream().foreach() -> map.foreach()
I was told that it is a flakey test that tests something that is no longer necessary.
resolving merge conflicts... |
@cmnbroad You're ok with this test being removed? Just wanted to double check before merging. |
@lbergelson Yes - this test is a leftover artifact of the original implementation, but we're no longer dependent on the features tested it so its fine to remove. |
closes #5533