Allow Options with defaults to be always non-required

[ymenager]

It would be useful to be able to programatically make a required field to "not required" before doing the parsing.

My use case is that I have defaults which are in a configuration file, so I'd like to be able to switch a field to non-required when a default value was saved in the config file

[ymenager]

Oh, actually I can see that in 3.x it's possible to do that by manipulating the CommandSpec... yay ! closing non-issue :)

[ymenager]

Actually i spoke too soon :-(

commandSpec.requiredArgs() is protected against modification, and there's no method that allows to remove something from that list. Would it be possible to make that modifiable ?

[ymenager]

Alternatively it would be nice for required to support default values, and not fail if a default value exists (also usage would need to support that to not include it's marker when default value exists).

This solution architecturally make actually make the more sense (if there's a default value then required should always be fulfilled)

[ymenager]

Ok, actually I've implemented that suggestion about allowing a field with a default value to never be required. I've made it optional through a parameter in @command so that picocli to avoid changing current behavior.

See pull request #267

[remkop]

@ymenager Very sorry for the late response. I must have accidentally switched off email notifications because I used to get emails for new issues but I did not for yours...

What you are saying makes sense. Options or positional parameters that have a value (either a user specified value or a default value) should not be the cause of a "missing required parameter" error.

I still need to take a closer look at your pull request but my first thought is that it is okay to change the current behaviour to the previous paragraph. If so, the @Command(notRequiredWithDefault) attribute would not be needed, which would simplify things.

[ymenager]

Sure I'm happy with it being the default behavior, I'm going to change the code in the PR branch accordingly

[remkop]

ok cool. Is it possible to separate this pull request from #262 ?

[ymenager]

Sure

[ymenager]

Just as note, one consequence of this change is that all primitives will never be "required" since they always have a default value

[remkop]

Thanks for pointing that out. That is not good...

It's getting a bit late for me so I need to sleep on it, but perhaps @Option and @Parameters need to have an additional attribute that provides the default value, so that picocli can verify that a default value has actually been specified.

This could be a simple defaultValue = "..." String attribute, and/or a defaultValueProvider = Class<? extends IDefaultValueProvider> attribute, where we introduce an additional interface, something like the below:

interface IDefaultValueProvider {
    /** Returns the default value for an option or positional parameter.
     * The returned value is converted to the type of the option/positional parameter
     * via the same type converter used when populating this option/positional
     * parameter from a command line argument.
     */
    String defaultValue();
}

Thoughts?

[ymenager]

Yeah, i noticed when a whole bunch of the existing tests broke after i made those changes lol

Yeah that would work.. I would say both defaultValue and defaultValueProvider would give extra flexibility to handle all use cases. Ideally IDefaultValueProvider should be able to access the field value so the same behavior as today (using the field's value) could be implemented

[remkop]

Something like this?

interface IDefaultValueProvider {
    /** Returns the default value for an option or positional parameter.
     * The returned value is converted to the type of the option/positional parameter
     * via the same type converter used when populating this option/positional
     * parameter from a command line argument.
     * @param previousValue the current value of the option or positional parameter, may be {@code null}
     */
    String defaultValue(Object previousValue);
}

[ymenager]

Nope... I would give some way to get to the command object as well as to the field

[ymenager]

like

interface IDefaultValueProvider {
    /** Returns the default value for an option or positional parameter.
     * The returned value is converted to the type of the option/positional parameter
     * via the same type converter used when populating this option/positional
     * parameter from a command line argument.
     * @param previousValue the current value of the option or positional parameter, may be {@code null}
     */
    String defaultValue(OptionSpec optionSpec);
}

[remkop]

Yes that is better. Perhaps pass in an ArgSpec so it also works for positional parameters.

And the defaultProvider attribute should perhaps be an attribute of @Command so you can specify it just once on the command instead of needing to specify it for each option/positional parameter.

In addition to declarative, users may also want to set this programmatically on the CommandLine.

[ymenager]

best have it both on @command as well as @parameter / @option (the later two being given priority over the prior)

[remkop]

I think it’d be good to start by adding a defaultProvider attribute to @Command and give programmatic access. That keeps the API small while giving users a lot of power.

If someone has a good use case for having a separate DefaultProvider class on a per-option basis we can add add this attribute to @Option and @Parameters.

I do wonder whether the IDefaultValueProvider should be generic to prepare for that eventuality:

interface IDefaultValueProvider<T extends ArgSpec> {
    String defaultValue(T argSpec);
}

[ymenager]

yup sounds good

[remkop]

Would you be interested in creating a pull request for this? (Ideally a new PR without the masking stuff.)

[ymenager]

Sure but not sure when I'll have time, my todo list is quite large at the moment

[remkop]

Understood. FYI, I've started to work on #262.

[ymenager]

K cool, I'll let you know when i have some time to help out

[remkop]

This is now implemented: OptionSpec and PositionalParamSpec objects with a non-null value for their defaultValue attribute are automatically not required.

Note that I split out separate tickets for adding a defaultValue attribute to the annotations API (#322) and for supporting a IDefaultValueProvider (#321).