321 add support for default value provider #458
Conversation
Following what was done for Command version provider I added a default value provider.
Following what was done for Command version provider I added a default value provider as proposed in remkop#321.
Codecov Report
@@ Coverage Diff @@
## master #458 +/- ##
============================================
- Coverage 88.63% 88.53% -0.11%
- Complexity 274 275 +1
============================================
Files 4 4
Lines 3767 3785 +18
Branches 892 897 +5
============================================
+ Hits 3339 3351 +12
- Misses 218 222 +4
- Partials 210 212 +2
Continue to review full report at Codecov.
|
| * Returns default value for a command, option or parameter. | ||
| * @return default value | ||
| * @throws Exception an exception detailing what went wrong when obtaining default value | ||
| */ |
There was a problem hiding this comment.
#321 has some javadoc for this method that I would like to use.
| * @return default value | ||
| * @throws Exception an exception detailing what went wrong when obtaining default value | ||
| */ | ||
| String defaultValue(ArgSpec argSpec) throws Exception; |
There was a problem hiding this comment.
The API in #321 did not have throws Exception in the signature, but I'm okay with this. I hadn't thought about this, but I guess it is convenient for implementors if this method is declared as throws Exception. I believe it is used in methods that currently also throw Exception so there is no additional error handling required there, which is nice.
There was a problem hiding this comment.
I admit I mimicked the way it worked for version provider
| for (OptionSpec option : commandSpec.options()) { applyDefault(option, required); } | ||
| for (PositionalParamSpec positional : commandSpec.positionalParameters()) { applyDefault(positional, required); } | ||
| for (OptionSpec option : commandSpec.options()) { applyDefault(commandSpec.defaultValueProvider, option, required); } | ||
| for (PositionalParamSpec positional : commandSpec.positionalParameters()) { applyDefault(commandSpec.defaultValueProvider, positional, required); } |
There was a problem hiding this comment.
Small nit, but I'd prefer to call the commandSpec.defaultValueProvider() method instead of referencing the commandSpec.defaultValueProvider field where possible.
| @@ -3650,6 +3677,15 @@ public CommandSpec aliases(String... aliases) { | |||
| return this; | |||
| } | |||
|
|
|||
| /** Returns the default value provider for this command. | |||
| * @return the default value provider or {@code null}*/ | |||
| public IDefaultValueProvider defaultValueProvider() { return defaultValueProvider; } | |||
There was a problem hiding this comment.
Please add @since 3.6 to the javadoc
| /** Sets default value provider for this command. | ||
| * @param defaultValueProvider the default value provider to use, or {@code null}. | ||
| * @return this CommandSpec for method chaining */ | ||
| public CommandSpec defaultValueProvider(IDefaultValueProvider defaultValueProvider) { this.defaultValueProvider = defaultValueProvider; return this; } |
| * @param defaultValueProvider the default value provider to use, or {@code null}. | ||
| * @return this CommandSpec for method chaining */ | ||
| public CommandSpec defaultValueProvider(IDefaultValueProvider defaultValueProvider) { this.defaultValueProvider = defaultValueProvider; return this; } | ||
|
|
There was a problem hiding this comment.
I am wondering whether it would be a good idea to also add programmatic API for setting/getting the IDefaultProvider on the CommandLine facade class.
If we think this is a good idea, it should work like the other CommandLine.getX and setX methods: apply the new X value to all subcommands that are registered at the time of the call. (See for example setStopAtPositional, setToggleBooleanFlags etc).
According to PR remkop#458 review: - fixed javadoc and since version - added a setter in Command facade for default provider - replaced use of defaultValueProvider field by its getter
… value provider
|
PR looks good and tests look good. I may add some more tests but this covers a lot. Can I ask you to rebase on master? That will give a cleaner git history when I merge the branch. |
|
Isn't it better to squash and merge ? https://help.github.com/articles/about-pull-request-merges/#squash-and-merge-your-pull-request-commits |
|
Merged. Thank you very much! |
Fixed misbehaviour that was introduced in PR remkop#458, see remkop#321 Now default provider overrides default or initial values only if the return value of the provider is not null.
Adding support for a default value provider that could enable me to have those defaults loaded from a configuration file.
See issue #321