CLI-324 Make adding OptionGroups and Options to existing Options easier (apache#230)

* Fixes for CLI-324

* added tests

* reverted change of getOptionGroup to public and added documentation

* added test cases and javadoc to explain functioning

Author: Claudenw

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

@@ -54,6 +54,25 @@ public class Options implements Serializable {
     /** A map of the option groups */
     private final Map<String, OptionGroup> optionGroups = new LinkedHashMap<>();
 
+    /**
+     * Adds options to this option.  If any Option in {@code options} already exists
+     * in this Options an IllegalArgumentException is thrown
+     *
+     * @param options the options to add.
+     * @return The resulting Options instance.
+     * @since 1.7.0
+     */
+    public Options addOptions(final Options options) {
+        for (Option opt : options.getOptions()) {
+            if (hasOption(opt.getKey())) {
+                throw new IllegalArgumentException("Duplicate key: " + opt.getKey());
+            }
+            addOption(opt);
+        }
+        options.getOptionGroups().forEach(this::addOptionGroup);
+        return this;
+    }
+
     /**
      * Adds an option instance
      *
@@ -233,6 +252,11 @@ public OptionGroup getOptionGroup(final Option opt) {
      * @return a Collection of OptionGroup instances.
      */
     Collection<OptionGroup> getOptionGroups() {
+        /* The optionGroups map will have duplicates in the values() results.  We
+         * use the HashSet to filter out duplicates and return a collection of
+         * OpitonGroup.  The decision to return a Collection rather than a set
+         * was probably to keep symmetry with the getOptions() method.
+         */
         return new HashSet<>(optionGroups.values());
     }
 
@@ -250,7 +274,7 @@ public Collection<Option> getOptions() {
      *
      * @return read-only List of required options
      */
-    public List getRequiredOptions() {
+    public List<?> getRequiredOptions() {
         return Collections.unmodifiableList(requiredOpts);
     }
 

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

@@ -21,9 +21,15 @@ Licensed to the Apache Software Foundation (ASF) under one or more
 import static org.junit.Assert.assertNotNull;
 import static org.junit.Assert.assertTrue;
 import static org.junit.Assert.fail;
+import static org.junit.jupiter.api.Assertions.assertArrayEquals;
+import static org.junit.jupiter.api.Assertions.assertThrows;
 
 import java.util.ArrayList;
+import java.util.Arrays;
 import java.util.Collection;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Set;
 
 import org.junit.Test;
 
@@ -78,6 +84,93 @@ public void testGetOptionsGroups() {
         assertEquals(2, options.getOptionGroups().size());
     }
 
+    @Test
+    public void testAddOptions() {
+        final Options options = new Options();
+
+        final OptionGroup group1 = new OptionGroup();
+        group1.addOption(Option.builder("a").build());
+        group1.addOption(Option.builder("b").build());
+
+        options.addOptionGroup(group1);
+
+        options.addOption(Option.builder("X").build());
+        options.addOption(Option.builder("y").build());
+
+        final Options underTest = new Options();
+        underTest.addOptions(options);
+
+        assertEquals(options.getOptionGroups(), underTest.getOptionGroups());
+        assertArrayEquals(options.getOptions().toArray(), underTest.getOptions().toArray());
+    }
+
+    @Test
+    public void testAddOptions2X() {
+        final Options options = new Options();
+
+        final OptionGroup group1 = new OptionGroup();
+        group1.addOption(Option.builder("a").build());
+        group1.addOption(Option.builder("b").build());
+
+        options.addOptionGroup(group1);
+
+        options.addOption(Option.builder("X").build());
+        options.addOption(Option.builder("y").build());
+
+        assertThrows(IllegalArgumentException.class, () -> options.addOptions(options));
+    }
+
+    @Test
+    public void testAddConflictingOptions() {
+        final Options options1 = new Options();
+        final OptionGroup group1 = new OptionGroup();
+        group1.addOption(Option.builder("a").build());
+        group1.addOption(Option.builder("b").build());
+        options1.addOptionGroup(group1);
+        options1.addOption(Option.builder("x").build());
+        options1.addOption(Option.builder("y").build());
+
+        final Options options2 = new Options();
+        final OptionGroup group2 = new OptionGroup();
+        group2.addOption(Option.builder("x").type(Integer.class).build());
+        group2.addOption(Option.builder("b").type(Integer.class).build());
+        options2.addOptionGroup(group2);
+        options2.addOption(Option.builder("c").build());
+
+        assertThrows(IllegalArgumentException.class, () -> options1.addOptions(options2));
+    }
+
+    @Test
+    public void testAddNonConflictingOptions() {
+        final Options options1 = new Options();
+        final OptionGroup group1 = new OptionGroup();
+        group1.addOption(Option.builder("a").build());
+        group1.addOption(Option.builder("b").build());
+        options1.addOptionGroup(group1);
+        options1.addOption(Option.builder("x").build());
+        options1.addOption(Option.builder("y").build());
+
+        final Options options2 = new Options();
+        final OptionGroup group2 = new OptionGroup();
+        group2.addOption(Option.builder("c").type(Integer.class).build());
+        group2.addOption(Option.builder("d").type(Integer.class).build());
+        options2.addOptionGroup(group2);
+        options1.addOption(Option.builder("e").build());
+        options1.addOption(Option.builder("f").build());
+
+        Options underTest = new Options();
+        underTest.addOptions(options1);
+        underTest.addOptions(options2);
+
+        List<OptionGroup> expected = Arrays.asList(group1, group2);
+        assertTrue(expected.size() == underTest.getOptionGroups().size() && expected.containsAll(underTest.getOptionGroups()));
+        Set<Option> expectOpt = new HashSet<>();
+        expectOpt.addAll(options1.getOptions());
+        expectOpt.addAll(options2.getOptions());
+        assertEquals(8, expectOpt.size());
+        assertTrue(expectOpt.size() == underTest.getOptions().size() && expectOpt.containsAll(underTest.getOptions()));
+    }
+
     @Test
     public void testHelpOptions() {
         OptionBuilder.withLongOpt("long-only1");