Factor out ArgumentDefinition classes and refactor CommandLineArgumentParser. #133

cmnbroad · 2018-03-12T14:23:35Z

CommandLineArgumentParser refactoring. Extract classes for positional and named argument definitions with common ArgumentDefinition base class. All GATK and Picard tests have passed with these changes (Picard requires no source changes, GATK changes corresponding to the gatherFieldValuesOfType changesr are in broadinstitute/gatk#4523).

Fixes:

Additional fixes:

dangling mutex arguments are not reported/rejected in some cases (some doc tests had these and had to be changed accordingly)
positional arguments cannot be populated from an expansion file
tagged arguments cannot be populated from an expansion file
gatherFieldValuesOfType (now renamed gatherArgumentValuesOfType) didn’t return values from plugins or positional arguments
minor command line and usage display formatting issues

Prerequisite for:

Existing tests will be refactored is a separate PR.

…tParser.

cmnbroad · 2018-03-12T19:45:20Z

For @jonn-smith (or @droazen) or whoever can get to it.

jonn-smith

A few comments / minor changes.

Looks pretty good. There was a lot of code, so I'm a little worried I didn't see into some details.

I found the parts where you grab a base field collection and modify it in place a little confusing, but it makes sense.

src/main/java/org/broadinstitute/barclay/argparser/ArgumentDefinition.java

jonn-smith · 2018-03-19T20:34:20Z

src/main/java/org/broadinstitute/barclay/argparser/ArgumentDefinition.java

+            }
+        } catch (final Exception ex) {
+            // Note: I assume this catches Exception to handle the case where the type is not instantiable
+            // TODO: Can't we just throw when this happens ??


I'm wondering the same thing as this TODO. Should we still try to run if the field can't be set properly?

The parser tries to auto-create a collection instance of the declared type if the argument field doesn't already have one. If that fails, it catches Exception and falls back to trying to create an ArrayList. If not for the backward compatibility issue, I would require the coder to create the collection, and just let it throw. But I suspect there is code that relies on this behavior. I'll remove that TODO and fix the comment (I originally added it as a note to myself when I was refactoring).

jonn-smith · 2018-03-20T14:48:31Z

src/main/java/org/broadinstitute/barclay/argparser/ArgumentDefinition.java

+     * @return never {@code null}.
+     */
+    @SuppressWarnings({"unchecked","rawtypes"})
+    protected static String getOptions(final Class<?> clazz) {


Is it maybe worth having a way to include some example options for String arguments as well? The implementation would need to fill out this example string.

That might be a good idea, but I'd prefer not to introduce any additional new features is a separate PR.

Sounds good. Can you add this in as a new issue (if you haven't already)?

Done #144. For the record, I also changed the getOptions method from a static method to an instance method to make it easier to implement this request when we do so, and renamed it to getOptionsAsDisplayString to better reflect what it does.

src/main/java/org/broadinstitute/barclay/argparser/CommandLineArgumentParser.java

src/main/java/org/broadinstitute/barclay/argparser/NamedArgumentDefinition.java

src/main/java/org/broadinstitute/barclay/argparser/PositionalArgumentDefinition.java

src/test/java/org/broadinstitute/barclay/argparser/CollectionArgumentUnitTests.java

src/test/java/org/broadinstitute/barclay/argparser/CommandLineArgumentParserTest.java

src/test/java/org/broadinstitute/barclay/argparser/TaggedArgumentTest.java

codecov-io · 2018-10-15T12:52:55Z

Codecov Report

Merging #133 into master will increase coverage by 0.49%.
The diff coverage is 80.72%.

@@             Coverage Diff              @@
##             master     #133      +/-   ##
============================================
+ Coverage     76.07%   76.56%   +0.49%     
- Complexity      593      669      +76     
============================================
  Files            22       26       +4     
  Lines          2215     2296      +81     
  Branches        457      455       -2     
============================================
+ Hits           1685     1758      +73     
- Misses          352      366      +14     
+ Partials        178      172       -6

Impacted Files	Coverage Δ	Complexity Δ
...institute/barclay/argparser/CommandLineParser.java	`0% <ø> (-78.73%)`	`0 <0> (-17)`
...titute/barclay/argparser/TaggedArgumentParser.java	`85.1% <100%> (+0.16%)`	`22 <1> (+1)`	⬆️
...lay/argparser/LegacyCommandLineArgumentParser.java	`75.91% <50%> (-0.16%)`	`128 <6> (ø)`
...arclay/argparser/PositionalArgumentDefinition.java	`69.81% <69.81%> (ø)`	`11 <11> (?)`
...nstitute/barclay/argparser/ArgumentDefinition.java	`70.43% <70.43%> (ø)`	`26 <26> (?)`
...titute/barclay/help/DefaultDocWorkUnitHandler.java	`76.87% <77.77%> (+4.95%)`	`74 <24> (-1)`	⬇️
...ute/barclay/argparser/NamedArgumentDefinition.java	`78.59% <78.59%> (ø)`	`122 <122> (?)`
.../barclay/argparser/CommandLineParserUtilities.java	`83.33% <83.33%> (ø)`	`2 <2> (?)`
...e/barclay/argparser/CommandLineArgumentParser.java	`89.61% <93.26%> (+6.73%)`	`115 <92> (-68)`	⬇️
...titute/barclay/argparser/CommandLineException.java	`47.05% <0%> (-2.95%)`	`2% <0%> (ø)`
... and 4 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9417229...ef82b6e. Read the comment docs.

cmnbroad · 2018-10-15T13:56:25Z

@jonn-smith I think I responded to all your comments.

jonn-smith

One minor bookkeeping request (adding in an issue if it doesn't already exist for my comment in ArgumentDefinition.java).

After that it looks good to merge.

jonn-smith · 2018-10-15T14:45:18Z

src/main/java/org/broadinstitute/barclay/argparser/ArgumentDefinition.java

+     * @return never {@code null}.
+     */
+    @SuppressWarnings({"unchecked","rawtypes"})
+    protected static String getOptions(final Class<?> clazz) {


Sounds good. Can you add this in as a new issue (if you haven't already)?

src/main/java/org/broadinstitute/barclay/argparser/CommandLineArgumentParser.java

src/main/java/org/broadinstitute/barclay/argparser/LegacyCommandLineArgumentParser.java

src/main/java/org/broadinstitute/barclay/argparser/NamedArgumentDefinition.java

jonn-smith · 2018-10-16T15:17:43Z

@cmnbroad Looks good! Merge away.

Factor out ArgumentDefinition classes and refactor CommandLineArgumen…

075dc02

…tParser.

cmnbroad requested a review from jonn-smith March 12, 2018 19:44

jonn-smith self-assigned this Mar 19, 2018

jonn-smith requested changes Mar 21, 2018

View reviewed changes

jonn-smith assigned cmnbroad and unassigned jonn-smith Mar 21, 2018

cmnbroad mentioned this pull request Apr 13, 2018

Proposal: use java bean validation (JSR 380) for constraint arguments #134

Open

cmnbroad added 2 commits October 11, 2018 10:42

Fix plugin args with some doc refactoring.

ab414e5

Code review comments.

ab7edfb

cmnbroad assigned jonn-smith Oct 15, 2018

jonn-smith approved these changes Oct 15, 2018

View reviewed changes

cmnbroad mentioned this pull request Oct 16, 2018

Allow @Argument annotations to declare valid option values #144

Open

Code review 2, plus fix some typos and remove dead code.

ef82b6e

cmnbroad merged commit 860ca67 into master Oct 18, 2018

cmnbroad deleted the cn_arg_refactor branch October 18, 2018 16:19

cmnbroad mentioned this pull request Oct 18, 2018

barclay should ALWAYS call setAccessible(true) when retrieving a Field object #66

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Factor out ArgumentDefinition classes and refactor CommandLineArgumentParser. #133

Factor out ArgumentDefinition classes and refactor CommandLineArgumentParser. #133

cmnbroad commented Mar 12, 2018 •

edited

Loading

cmnbroad commented Mar 12, 2018

jonn-smith left a comment

jonn-smith Mar 19, 2018

cmnbroad Mar 26, 2018 •

edited

Loading

jonn-smith Mar 20, 2018

cmnbroad Oct 15, 2018

jonn-smith Oct 15, 2018

cmnbroad Oct 16, 2018

codecov-io commented Oct 15, 2018 •

edited

Loading

cmnbroad commented Oct 15, 2018

jonn-smith left a comment

jonn-smith Oct 15, 2018

jonn-smith commented Oct 16, 2018

Factor out ArgumentDefinition classes and refactor CommandLineArgumentParser. #133

Factor out ArgumentDefinition classes and refactor CommandLineArgumentParser. #133

Conversation

cmnbroad commented Mar 12, 2018 • edited Loading

cmnbroad commented Mar 12, 2018

jonn-smith left a comment

Choose a reason for hiding this comment

jonn-smith Mar 19, 2018

Choose a reason for hiding this comment

cmnbroad Mar 26, 2018 • edited Loading

Choose a reason for hiding this comment

jonn-smith Mar 20, 2018

Choose a reason for hiding this comment

cmnbroad Oct 15, 2018

Choose a reason for hiding this comment

jonn-smith Oct 15, 2018

Choose a reason for hiding this comment

cmnbroad Oct 16, 2018

Choose a reason for hiding this comment

codecov-io commented Oct 15, 2018 • edited Loading

Codecov Report

cmnbroad commented Oct 15, 2018

jonn-smith left a comment

Choose a reason for hiding this comment

jonn-smith Oct 15, 2018

Choose a reason for hiding this comment

jonn-smith commented Oct 16, 2018

cmnbroad commented Mar 12, 2018 •

edited

Loading

cmnbroad Mar 26, 2018 •

edited

Loading

codecov-io commented Oct 15, 2018 •

edited

Loading