Address comments

broadinstitute · Nov 30, 2017 · d0a4dcf · d0a4dcf
1 parent f42fa6f
commit d0a4dcf
Show file tree

Hide file tree

Showing 2 changed files with 53 additions and 26 deletions.
diff --git a/src/main/java/org/broadinstitute/barclay/argparser/CommandLineArgumentParser.java b/src/main/java/org/broadinstitute/barclay/argparser/CommandLineArgumentParser.java
@@ -15,6 +15,7 @@
 import java.lang.reflect.Field;
 import java.lang.reflect.InvocationTargetException;
 import java.util.*;
+import java.util.function.Consumer;
 import java.util.stream.Collectors;
 import java.util.stream.Stream;
 
@@ -616,9 +617,8 @@ private void setArgument(ArgumentDefinition argumentDefinition, List<String> val
                 // if this is a collection then we only want to clear it once at the beginning, before we process
                 // any of the values, unless we're in APPEND_TO_COLLECTIONS mode, in which case we leave the initial
                 // and append to it
-                @SuppressWarnings("rawtypes")
-                final Collection c = (Collection) argumentDefinition.getFieldValue();
-                c.clear();
+                System.err.println("Clear collection");
+                applyToCollection(argumentDefinition, Collection::clear);
             }
             values = expandListFile(values);
         }
@@ -680,27 +680,35 @@ private void setArgument(ArgumentDefinition argumentDefinition, List<String> val
 
             // check the argument range
             checkArgumentRange(argumentDefinition, value);
+            // set the already parsed argument
+            setParsedArgumentValue(argumentDefinition, value);
+        }
+    }
 
-            if (argumentDefinition.isCollection) {
-                @SuppressWarnings("rawtypes")
-                final Collection c = (Collection) argumentDefinition.getFieldValue();
-                try {
-                    if (value == null) {
-                        //user specified this arg=null which is interpreted as empty list
-                        c.clear();
-                    } else {
-                        c.add(value);
-                    }
-                } catch (UnsupportedOperationException e) {
-                    throw new CommandLineException.CommandLineParserInternalException(String.format("Collection arguments seems immutable: \"%s\". Initialized collection should support the clear and add methods, but %s does not.",
-                            argumentDefinition.getLongName(),
-                            c.getClass().getName()));
-                }
-                argumentDefinition.hasBeenSet = true;
+    private void setParsedArgumentValue(final ArgumentDefinition argumentDefinition, final Object value) {
+        if (argumentDefinition.isCollection) {
+            if (value == null) {
+                applyToCollection(argumentDefinition, Collection::clear);
             } else {
-                argumentDefinition.setFieldValue(value);
-                argumentDefinition.hasBeenSet = true;
+                applyToCollection(argumentDefinition, c -> c.add(value));
             }
+            argumentDefinition.hasBeenSet = true;
+        } else {
+            argumentDefinition.setFieldValue(value);
+            argumentDefinition.hasBeenSet = true;
+        }
+    }
+
+    // apply a function to a collection, catching UnsuportedOperationException into a more informative
+    private void applyToCollection(final ArgumentDefinition argumentDefinitionCollection, final Consumer<Collection> function) {
+
+        final Collection c = (Collection) argumentDefinitionCollection.getFieldValue();
+        try {
+            function.accept(c);
+        } catch (UnsupportedOperationException e) {
+            throw new CommandLineException.CommandLineParserInternalException(String.format("Collection arguments seems immutable: \"%s\". Initialized collection should support the clear and add methods, but %s does not.",
+                    argumentDefinitionCollection.getLongName(),
+                    c.getClass().getName()));
         }
     }
 

diff --git a/src/test/java/org/broadinstitute/barclay/argparser/CollectionArgumentUnitTests.java b/src/test/java/org/broadinstitute/barclay/argparser/CollectionArgumentUnitTests.java
@@ -278,14 +278,33 @@ public void testCollectionThatCannotBeAutoInitialized() {
     }
 
     class ImmutableCollectionArguments {
-        @Argument
-        public List<String> LIST = Collections.emptyList();
+        @Argument(optional = true)
+        public List<String> LIST = Arrays.asList("T");
     }
 
-    @Test(expectedExceptions = CommandLineException.CommandLineParserInternalException.class)
-    public void testCollectionThatIsImmmutable() {
+    @DataProvider
+    public Object[][] argsForImmmutableCollectionTest() {
+        return new Object[][] {
+                // replace mode
+                {Collections.EMPTY_SET,                                                 // parser options
+                        new String[]{"--LIST", "A"}},                                   // arguments (real)
+                // replace mode
+                {Collections.EMPTY_SET,                                                 // parser options
+                        new String[]{"--LIST", "null"}},                                // arguments (null)
+
+                // append mode
+                {Collections.singleton(CommandLineParserOptions.APPEND_TO_COLLECTIONS), // parser options
+                        new String[]{"--LIST", "A"}},                                   // arguments (real)
+                // append mode
+                {Collections.singleton(CommandLineParserOptions.APPEND_TO_COLLECTIONS), // parser options
+                        new String[]{"--LIST", "null"}},                                // arguments (null)
+        };
+    }
+
+    @Test(dataProvider = "argsForImmmutableCollectionTest", expectedExceptions = CommandLineException.CommandLineParserInternalException.class)
+    public void testCollectionThatIsImmmutable(final Set<CommandLineParserOptions> opts, final String[] args) {
         final ImmutableCollectionArguments o = new ImmutableCollectionArguments();
-        new CommandLineArgumentParser(o).parseArguments(System.err, new String[]{"--LIST", "A"});
+        new CommandLineArgumentParser(o, Collections.emptyList(), opts).parseArguments(System.err, args);
     }
 
     //////////////////////////////////////////////////////////////////