garydgregory commented on code in PR #230:
URL: https://github.com/apache/commons-cli/pull/230#discussion_r1491174913
##########
src/main/java/org/apache/commons/cli/Options.java:
##########
@@ -54,6 +54,18 @@ public class Options implements Serializable {
/** A map of the option groups */
private final Map<String, OptionGroup> optionGroups = new
LinkedHashMap<>();
+ /**
+ * Adds options to this option.
+ * @param options the options to add.
+ * @return this for chaining.
Review Comment:
We do not need to state the purpose of returning `this` as fluent style
coding since the return value can be used for _anything_.
##########
src/test/java/org/apache/commons/cli/OptionsTest.java:
##########
@@ -78,6 +79,26 @@ 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);
Review Comment:
You need a test that calls `addOptions()` twice with different options to
verify that the calls are cumulative and do not replace what's already there.
Also: What should happen if I add the same options twice? For example:
```
underTest.addOptions(options);
underTest.addOptions(options);
```
Also test adding an empty Options object to make sure nothing odd happens.
Edge cases, edge cases... ;-)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscr...@commons.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
