#252 Ambiguity in OptionMetadataBuilder default value setter

bootique · Nov 29, 2018 · 1b78f39 · 1b78f39
1 parent d7a4db6
commit 1b78f39
Show file tree

Hide file tree

Showing 5 changed files with 33 additions and 22 deletions.
diff --git a/RELEASE-NOTES.md b/RELEASE-NOTES.md
@@ -21,6 +21,7 @@
 * #249 Replace confusing variants of addOption in BQCoreModuleExtender
 * #250 Update modules parent to 0.14
 * #251 Rename method addConfigOnOption in BQCoreModuleExtender
+* #252 Ambiguity in OptionMetadataBuilder default value setter 
 
 ## 0.25 
 

diff --git a/UPGRADE.md b/UPGRADE.md
@@ -22,6 +22,9 @@
 
 ## 1.0.RC1
 
+* [bootique #251](https://github.com/bootique/bootique/issues/249): Method `defaultValue()` in OptionMetadata builder 
+changed to `valueOptionalWithDefault()`
+
 * [bootique #251](https://github.com/bootique/bootique/issues/249): Method `addConfigOnOption(String optionName, String configResourceId)`
 in `BQCoreModuleExtender` renamed to `mapConfigResource`
 

diff --git a/bootique/src/main/java/io/bootique/jopt/JoptCliFactory.java b/bootique/src/main/java/io/bootique/jopt/JoptCliFactory.java
@@ -122,25 +122,19 @@ protected void addOption(OptionParser parser, OptionMetadata option) {
         // TODO: how do we resolve short name conflicts?
         List<String> longAndShort = asList(option.getShortName(), option.getName());
         OptionSpecBuilder optionBuilder = parser.acceptsAll(longAndShort, description);
-        ArgumentAcceptingOptionSpec<String> optionSpec = null;
         switch (option.getValueCardinality()) {
             case OPTIONAL:
-                optionSpec = optionBuilder.withOptionalArg().describedAs(option.getValueName());
+                ArgumentAcceptingOptionSpec<String> optionSpec = optionBuilder.withOptionalArg().describedAs(option.getValueName());
+                if(option.getDefaultValue() != null) {
+                    optionSpec.defaultsTo(option.getDefaultValue());
+                }
                 break;
             case REQUIRED:
-                optionSpec = optionBuilder.withRequiredArg().describedAs(option.getValueName());
+                optionBuilder.withRequiredArg().describedAs(option.getValueName());
                 break;
             default:
                 break;
         }
-
-        if(option.getDefaultValue() != null) {
-            if(optionSpec == null) {
-                // need optional arg to enable default value
-                optionSpec = optionBuilder.withOptionalArg();
-            }
-            optionSpec.defaultsTo(option.getDefaultValue());
-        }
     }
 
     // using option-bound command strategy...

diff --git a/bootique/src/main/java/io/bootique/meta/application/OptionMetadata.java b/bootique/src/main/java/io/bootique/meta/application/OptionMetadata.java
@@ -130,16 +130,29 @@ public Builder valueOptional(String valueName) {
         }
 
         /**
-         * Sets the default value for this option that will be used if the option is provided on
+         * Marks value optional and sets the default value for this option that will be used if the option is provided on
          * command line without an explicit value.
          *
          * @param defaultValue a default value for the option.
          * @return this builder instance
-         * @since 0.24
+         * @since 1.0.RC1
          */
-        public Builder defaultValue(String defaultValue) {
+        public Builder valueOptionalWithDefault(String defaultValue) {
+            return valueOptionalWithDefault("", defaultValue);
+        }
+
+        /**
+         * Marks value optional and sets the default value for this option that will be used if the option is provided on
+         * command line without an explicit value.
+         *
+         * @param valueName a description of value
+         * @param defaultValue a default value for the option.
+         * @return this builder instance
+         * @since 1.0.RC1
+         */
+        public Builder valueOptionalWithDefault(String valueName, String defaultValue) {
             this.option.defaultValue = defaultValue;
-            return this;
+            return valueOptional(valueName);
         }
 
         public OptionMetadata build() {

diff --git a/bootique/src/test/java/io/bootique/Bootique_CliOptionsIT.java b/bootique/src/test/java/io/bootique/Bootique_CliOptionsIT.java
@@ -178,7 +178,7 @@ public void testOption_OverrideConfig() {
                 .module(binder -> BQCoreModule
                         .extend(binder)
                         .addOptions(OptionMetadata.builder("opt-1").valueOptional().build(),
-                                OptionMetadata.builder("opt-2").valueOptional().defaultValue("2").build())
+                                OptionMetadata.builder("opt-2").valueOptionalWithDefault("2").build())
                         .mapConfigPath("opt-1", "c.m.l")
                         .mapConfigPath("opt-2", "c.m.k"))
                 .createRuntime();
@@ -208,7 +208,7 @@ public void testOptionPathAbsentInYAML() {
     public void testOptionsCommandAndModuleOverlapping() {
         BQRuntime runtime = runtimeFactory.app("--config=classpath:io/bootique/config/test4.yml", "--cmd-1", "--opt-1")
                 .module(binder -> BQCoreModule.extend(binder)
-                        .addOption(OptionMetadata.builder("opt-1").valueOptional().defaultValue("2").build())
+                        .addOption(OptionMetadata.builder("opt-1").valueOptionalWithDefault("2").build())
                         .mapConfigPath("opt-1", "c.m.k")
                         .addCommand(new TestOptionCommand1()))
                 .createRuntime();
@@ -248,8 +248,8 @@ public void testOptionsWithOverlappingPath_OverrideConfig() {
         BQRuntime runtime = runtimeFactory.app("--config=classpath:io/bootique/config/test4.yml", "--opt-2", "--opt-3")
                 .module(binder -> BQCoreModule.extend(binder)
                         .addOptions(OptionMetadata.builder("opt-1").valueOptional().build(),
-                                OptionMetadata.builder("opt-2").valueOptional().defaultValue("2").build(),
+                                OptionMetadata.builder("opt-2").valueOptionalWithDefault("2").build(),
-                                OptionMetadata.builder("opt-3").valueOptional().defaultValue("3").build())
+                                OptionMetadata.builder("opt-3").valueOptionalWithDefault("3").build())
                         .mapConfigPath("opt-1", "c.m.k")
                         .mapConfigPath("opt-2", "c.m.k")
                         .mapConfigPath("opt-3", "c.m.k"))
@@ -301,7 +301,7 @@ public void testMultipleOptionsConfigFiles_OverrideInCLIOrder() {
     public void testOptionDefaultValue() {
         BQRuntime runtime = runtimeFactory.app("--option")
                 .module(b -> BQCoreModule.extend(b).addOptions(
-                        OptionMetadata.builder("option").defaultValue("val").build()
+                        OptionMetadata.builder("option").valueOptionalWithDefault("val").build()
                 ))
                 .createRuntime();
         Cli cli = runtime.getInstance(Cli.class);
@@ -313,7 +313,7 @@ public void testOptionDefaultValue() {
     public void testMissingOptionDefaultValue() {
         BQRuntime runtime = runtimeFactory.app()
                 .module(b -> BQCoreModule.extend(b).addOptions(
-                        OptionMetadata.builder("option").defaultValue("val").build()
+                        OptionMetadata.builder("option").valueOptionalWithDefault("val").build()
                 ))
                 .createRuntime();
         Cli cli = runtime.getInstance(Cli.class);
@@ -442,7 +442,7 @@ static final class CommandWithDefaultOptionValue extends CommandWithMetadata {
 
         public CommandWithDefaultOptionValue() {
             super(CommandMetadata.builder("cmd")
-                    .addOption(OptionMetadata.builder("option").defaultValue("val").build()));
+                    .addOption(OptionMetadata.builder("option").valueOptionalWithDefault("val").build()));
         }
 
         @Override