From d0a4dcfe0c6204600912d64efc4ce7a3de5cdd8f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20G=C3=B3mez-S=C3=A1nchez?= Date: Mon, 30 Oct 2017 15:00:47 +0100 Subject: [PATCH] Address comments --- .../argparser/CommandLineArgumentParser.java | 50 +++++++++++-------- .../CollectionArgumentUnitTests.java | 29 +++++++++-- 2 files changed, 53 insertions(+), 26 deletions(-) diff --git a/src/main/java/org/broadinstitute/barclay/argparser/CommandLineArgumentParser.java b/src/main/java/org/broadinstitute/barclay/argparser/CommandLineArgumentParser.java index c83386b9..f0198aa2 100644 --- 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 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 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 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 index 16252abd..6613c2a9 100644 --- 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 LIST = Collections.emptyList(); + @Argument(optional = true) + public List 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 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); } //////////////////////////////////////////////////////////////////