Bugfix/cli 221 add Option.Builder.listValueSeparator()

src/main/java/org/apache/commons/cli/Char.java

@@ -40,6 +40,9 @@ final class Char {
     /** Tab. */
     static final char TAB = '\t';
 
+    /** Comma. */
+    static final char COMMA = ',';
+
     private Char() {
         // empty
     }

src/main/java/org/apache/commons/cli/DefaultParser.java

@@ -617,6 +617,9 @@ private void handleToken(final String token) throws ParseException {
                 skipParsing = true;
             } else if (currentOption != null && currentOption.acceptsArg() && isArgument(token)) {
                 currentOption.processValue(stripLeadingAndTrailingQuotesDefaultOn(token));
+                if (currentOption.isValueSeparatorUsedForSingleArgument()) {
+                    currentOption = null;
+                }
             } else if (token.startsWith("--")) {
                 handleLongOption(token);
             } else if (token.startsWith("-") && !"-".equals(token)) {

src/main/java/org/apache/commons/cli/Option.java

@@ -103,6 +103,9 @@ private static Class<?> toType(final Class<?> type) {
         /** The character that is the value separator. */
         private char valueSeparator;
 
+        /** Multiple values are within a single argument separated by valueSeparator char */
+        private boolean valueSeparatorUsedForSingleArgument;
+
         /**
          * Constructs a new {@code Builder} with the minimum required parameters for an {@code Option} instance.
          *
@@ -326,7 +329,9 @@ public Builder valueSeparator() {
         }
 
         /**
-         * The Option will use {@code sep} as a means to separate argument values.
+         * The Option will use {@code sep} as a means to separate java-property-style argument values.
+         *
+         * Method is mutually exclusive to listValueSeparator() method.
          * <p>
          * <strong>Example:</strong>
          * </p>
@@ -342,6 +347,10 @@ public Builder valueSeparator() {
          * String propertyValue = line.getOptionValues("D")[1]; // will be "value"
          * </pre>
          *
+         * In the above example, followup arguments are interpreted
+         * to be additional values to this option, needs to be terminated with -- so that
+         * others options or args can follow.
+         *
          * @param valueSeparator The value separator.
          * @return this builder.
          */
@@ -350,6 +359,51 @@ public Builder valueSeparator(final char valueSeparator) {
             return this;
         }
 
+        /**
+         * The Option will use ',' to invoke listValueSeparator()
+         *
+         * @return this builder.
+         * @since 1.11.0
+         */
+        public Builder listValueSeparator() {
+            return listValueSeparator(Char.COMMA);
+        }
+
+        /**
+         * Defines the separator used to split a list of values passed in a single argument.
+         *
+         * Method is mutually exclusive to valueSeparator() method. In the resulting option,
+         * isValueSeparatorUsedForSingleArgument() will return true.
+         *
+         * <p>
+         * <strong>Example:</strong>
+         * </p>
+         *
+         * <pre>
+         * final Option colors = Option.builder().option("c").hasArgs().listValueSeparator('|').build();
+         * final Options options = new Options();
+         * options.addOption(colors);
+         *
+         * final String[] args = {"-c", "red|blue|yellow", "b,c"};
+         * final DefaultParser parser = new DefaultParser();
+         * final CommandLine commandLine = parser.parse(options, args, null, true);
+         * final String [] colorValues = commandLine.getOptionValues(colors);
+         * // colorValues[0] will be "red"
+         * // colorValues[1] will be "blue"
+         * // colorValues[2] will be "yellow"
+         * final String arguments = commandLine.getArgs()[0]; // will be b,c
+         *
+         * </pre>
+         *
+         * @param listValueSeparator The char to be used to split the argument into mulitple values.
+         * @return this builder.
+         * @since 1.11.0
+         */
+        public Builder listValueSeparator(final char listValueSeparator) {
+            this.valueSeparator = listValueSeparator;
+            this.valueSeparatorUsedForSingleArgument = true;
+            return this;
+        }
     }
 
     /** Empty array. */
@@ -430,6 +484,9 @@ public static Builder builder(final String option) {
     /** The character that is the value separator. */
     private char valueSeparator;
 
+    /** Multiple values are within a single argument separated by valueSeparator char */
+    private boolean valueSeparatorUsedForSingleArgument;
+
     /**
      * Private constructor used by the nested Builder class.
      *
@@ -452,6 +509,7 @@ private Option(final Builder builder) {
         this.type = builder.type;
         this.valueSeparator = builder.valueSeparator;
         this.converter = builder.converter;
+        this.valueSeparatorUsedForSingleArgument = builder.valueSeparatorUsedForSingleArgument;
     }
 
     /**
@@ -834,6 +892,27 @@ public boolean isRequired() {
         return required;
     }
 
+    /**
+     * Tests whether multiple values are expected in a single argument split by a separation character
+     *
+     * @return boolean true when the builder's listValueSeparator() method was used. Multiple values are expected in a single argument and
+     *                 are split by a separation character.
+     * @since 1.11.0
+     */
+    public boolean isValueSeparatorUsedForSingleArgument() {
+        return valueSeparatorUsedForSingleArgument;
+    }
+
+    /**
+     * Set this to true to use the valueSeparator only on a single argument. See also builder's listValueSeparator() method.
+     *
+     * @param valueSeparatorUsedForSingleArgument the new value for this property
+     * @since 1.11.0
+     */
+    public void setValueSeparatorUsedForSingleArgument(final boolean valueSeparatorUsedForSingleArgument) {
+        this.valueSeparatorUsedForSingleArgument = valueSeparatorUsedForSingleArgument;
+    }
+
     /**
      * Processes the value. If this Option has a value separator the value will have to be parsed into individual tokens. When n-1 tokens have been processed
      * and there are more value separators in the value, parsing is ceased and the remaining characters are added as a single token.

src/test/java/org/apache/commons/cli/DefaultParserTest.java

@@ -35,6 +35,7 @@ Licensed to the Apache Software Foundation (ASF) under one or more
 import org.junit.jupiter.params.provider.Arguments;
 import org.junit.jupiter.params.provider.ArgumentsProvider;
 import org.junit.jupiter.params.provider.ArgumentsSource;
+import org.junit.jupiter.params.provider.ValueSource;
 
 class DefaultParserTest extends AbstractParserTestCase {
 
@@ -400,6 +401,96 @@ void testParseSkipNonHappyPath() throws ParseException {
         assertTrue(e.getMessage().contains("-d"));
     }
 
+    @Test
+    void legacyStopAtNonOption() throws ParseException {
+        final Option a = Option.builder().option("a").longOpt("first-letter").build();
+        final Option b = Option.builder().option("b").longOpt("second-letter").build();
+        final Option c = Option.builder().option("c").longOpt("third-letter").build();
+
+        final Options options = new Options();
+        options.addOption(a);
+        options.addOption(b);
+        options.addOption(c);
+
+        final String[] args = {"-a", "-b", "-c", "-d", "arg1", "arg2"}; // -d is rogue option
+
+        final DefaultParser parser = new DefaultParser();
+
+        final CommandLine commandLine = parser.parse(options, args, null, true);
+        assertEquals(3, commandLine.getOptions().length);
+        assertEquals(3, commandLine.getArgs().length);
+        assertTrue(commandLine.getArgList().contains("-d"));
+        assertTrue(commandLine.getArgList().contains("arg1"));
+        assertTrue(commandLine.getArgList().contains("arg2"));
+
+        final UnrecognizedOptionException e = assertThrows(UnrecognizedOptionException.class, () -> parser.parse(options, args, null, false));
+        assertTrue(e.getMessage().contains("-d"));
+    }
+
+    @Test
+    void listValueSeparatorTest() throws ParseException {
+        final Option colors = Option.builder().option("c").longOpt("colors").hasArgs().listValueSeparator('|').build();
+        final Options options = new Options();
+        options.addOption(colors);
+
+        final String[] args = {"-c", "red|blue|yellow", "b,c"};
+        final DefaultParser parser = new DefaultParser();
+        final CommandLine commandLine = parser.parse(options, args, null, true);
+        final String [] colorValues = commandLine.getOptionValues(colors);
+        assertEquals(3, colorValues.length);
+        assertEquals("red", colorValues[0]);
+        assertEquals("blue", colorValues[1]);
+        assertEquals("yellow", colorValues[2]);
+        assertEquals("b,c", commandLine.getArgs()[0]);
+    }
+
+    @ParameterizedTest
+    @ValueSource(strings = {
+            "--colors=red,blue,yellow b",
+            "--colors red,blue,yellow b",
+            "-c=red,blue,yellow b",
+            "-c red,blue,yellow b"})
+    void listValueSeparatorDefaultTest(final String args) throws ParseException {
+        final Option colors = Option.builder().option("c").longOpt("colors").hasArgs().listValueSeparator().build();
+        final Options options = new Options();
+        options.addOption(colors);
+
+        final DefaultParser parser = new DefaultParser();
+        final CommandLine commandLine = parser.parse(options, args.split(" "), null, true);
+        final String [] colorValues = commandLine.getOptionValues(colors);
+        assertEquals(3, colorValues.length);
+        assertEquals("red", colorValues[0]);
+        assertEquals("blue", colorValues[1]);
+        assertEquals("yellow", colorValues[2]);
+        assertEquals("b", commandLine.getArgs()[0]);
+    }
+
+    @ParameterizedTest
+    @ValueSource(strings = {
+            "--colors=red,blue,yellow -f bar b",
+            "-f bar --colors=red,blue,yellow b",
+            "b --colors=red,blue,yellow -f bar",
+            "b --colors=red -c blue --colors=yellow -f bar",
+        })
+    void listValueSeparatorSeriesDoesntMatter(final String args) throws ParseException {
+        final Option colors = Option.builder().option("c").longOpt("colors").hasArgs().listValueSeparator().build();
+        final Option foo = Option.builder().option("f").hasArg().build();
+        final Options options = new Options();
+        options.addOption(colors);
+        options.addOption(foo);
+
+        final DefaultParser parser = new DefaultParser();
+        final CommandLine commandLine = parser.parse(options, args.split(" "), null, false);
+        final String [] colorValues = commandLine.getOptionValues(colors);
+        final String fooValue = commandLine.getOptionValue(foo);
+        assertEquals(3, colorValues.length);
+        assertEquals("red", colorValues[0]);
+        assertEquals("blue", colorValues[1]);
+        assertEquals("yellow", colorValues[2]);
+        assertEquals("bar", fooValue);
+        assertEquals("b", commandLine.getArgs()[0]);
+    }
+
     @Override
     @Test
     @Disabled("Test case handled in the parameterized tests as \"DEFAULT behavior\"")

src/test/java/org/apache/commons/cli/OptionTest.java

@@ -357,4 +357,34 @@ void testTypeObject() {
         option.setType(type);
         assertEquals(CharSequence.class, option.getType());
     }
+
+    @Test
+    void testDefaultValueSeparator() {
+        final Option option = Option.builder().option("a").hasArgs().valueSeparator().build();
+        assertFalse(option.isValueSeparatorUsedForSingleArgument());
+        assertTrue(option.hasValueSeparator());
+        assertEquals('=', option.getValueSeparator());
+    }
+
+    @Test
+    void testDefaultListValueSeparator() {
+        final Option option = Option.builder().option("a").hasArgs().listValueSeparator().build();
+        assertTrue(option.isValueSeparatorUsedForSingleArgument());
+        assertTrue(option.hasValueSeparator());
+        assertEquals(',', option.getValueSeparator());
+    }
+
+    @Test
+    void testListValueSeparator() {
+        final Option option = Option.builder().option("a").hasArgs().listValueSeparator('|').build();
+        assertTrue(option.isValueSeparatorUsedForSingleArgument());
+        assertTrue(option.hasValueSeparator());
+        assertEquals('|', option.getValueSeparator());
+
+        option.setValueSeparatorUsedForSingleArgument(false);
+        assertFalse(option.isValueSeparatorUsedForSingleArgument());
+        assertTrue(option.hasValueSeparator());
+        assertEquals('|', option.getValueSeparator());
+
+    }
 }