Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support default value and custom converter for individual options/parameters #1308

Closed
asarkar opened this issue Jan 12, 2021 · 12 comments
Closed

Comments

@asarkar
Copy link

asarkar commented Jan 12, 2021

Currently, the default value and custom converter can only be specified at the class level, which is not useful at all for the following reasons:

  1. IDefaultValueProvider returns a String, not the actual field type.
  2. It is cumbersome to use IDefaultValueProvider to provide default values for multiple fields.
  3. Custom converter is useless when the same source type (string) needs to be converted to different target types.

Being able to provide a default value provider class and a converter class in the Option (and Parameter) annotation would be extremely useful. The default value provider may return a type that can be converted to the field type using the available converters (including the inline one).

@remkop
Copy link
Owner

remkop commented Jan 13, 2021

Hi @asarkar, thanks for your feedback!

Are you aware that the @Option and @Parameters annotation have a converter attribute where applications can specify a different type converter for each option/positional parameter?

The user manual has a section Option-specific Type Converters but I realize now that this may be easily overlooked. Thanks for pointing that out! I will improve the user manual shortly.

About default values, the @Option and @Parameters annotations have a defaultValue attribute. This is a String, like you pointed out, and just like a command line argument String, the defaultValue string will be converted to the option type by the option's ITypeConverter. (I think it already works like you are suggesting.)

Maybe interesting: the defaultValue attribute may contain a variable, and this variable can be defined not only in system properties or environment variables, but also in the resource bundle for the command. So this is an alternative way to dynamically determine default values for options and positional parameters.

The other way to dynamically provide default values is to set a IDefaultValueProvider on the command. I had not considered making it possible to specify a separate IDefaultValueProvider for each option/positional parameter. Are you aware that picocli provides a built-in PropertiesDefaultProvider, and would this meet your requirements?

Is this useful?
(I got the impression you may have missed the existence of the converter attribute, but I am not sure if that answers all your questions...)

@asarkar
Copy link
Author

asarkar commented Jan 13, 2021

Hi @remkop
You're right that I had missed the option-specific converter. However, the default value string doesn't work for the following.

@CommandLine.Option(names = {"--kubeconfig"}, description = "kubeconfig")
public void setKubeconfig(Path kubeconfig) {
    if (kubeconfig == null) {
        String jar = MyCli.class.getProtectionDomain().getCodeSource().getLocation().getPath();
        if (jar != null && jar.endsWith(".jar")) {
            this.kubeconfig = extractKubeconfig(
                    URLDecoder.decode(jar, StandardCharsets.UTF_8), String.join(".", context, namespace)
            );
            if (this.kubeconfig == null) {
                this.kubeconfig = extractKubeconfig(URLDecoder.decode(jar, StandardCharsets.UTF_8), "config");
            }
        }
        if (this.kubeconfig == null) {
            this.kubeconfig = Paths.get("config");
        }
    } else {
        this.kubeconfig = kubeconfig;
    }
    if (!this.kubeconfig.isAbsolute()) {
        this.kubeconfig = Paths.get(System.getProperty("user.home"))
                .resolve(".kube")
                .resolve(this.kubeconfig);
    }
    if (!Files.isRegularFile(this.kubeconfig) || !Files.isReadable(this.kubeconfig)) {
        throw new CommandLine.ParameterException(
                spec.commandLine(),
                "<kubeconfig> doesn't exist or isn't readable"
        );
    }
    LOG.info("Using kubeconfig: {}", this.kubeconfig.toAbsolutePath());
}

Obviously, the above can't be achieved by a simple string. To make things worse, the setter method is not called if the option is not specified by the user (null), so I end up calling it from Callable.call myself. That's where an option-specific default type provider would come really handy.

@remkop
Copy link
Owner

remkop commented Jan 13, 2021

Sorry I don’t follow.
Why not put all the logic in that setter method into a custom type converter for that option?

@remkop
Copy link
Owner

remkop commented Jan 13, 2021

Then make the default value a special string (instead of null), so the type converter can have special logic for the “no value specified” case.

@remkop
Copy link
Owner

remkop commented Jan 13, 2021

Actually, thinking about it some more, you don’t need a custom type converter, the logic in the setter method is fine.

All you need is to set a defaultValue = "__Not_specified" sentinel value so you can check for a Path with that value instead of null. That way you don’t need to invoke the setter from the Callable.call method any more.

@asarkar
Copy link
Author

asarkar commented Jan 13, 2021

Just so that I’m clear, using the defaultValue attribute in the annotation forces the setter being called even if the user doesn’t specify that option? I’ll try that and close the ticket if that works. It might be useful to document it though in the section for default values.

@remkop
Copy link
Owner

remkop commented Jan 13, 2021

using the defaultValue attribute in the annotation forces the setter being called even if the user doesn’t specify that option?

Yes, that is correct. 👍

I am always open to suggestions for improving the documentation. 🔧

@asarkar
Copy link
Author

asarkar commented Jan 13, 2021

Verified to be working using a default value "null" (that's a string, not the null value). I'm leaving this ticket open in case you want to use it for improving the docs (seems there are two instances that came up in the discussions). Otherwise, feel free to close.
Thanks for the help.

@remkop
Copy link
Owner

remkop commented Jan 13, 2021

@asarkar Good to hear you found something that works for you!

Don't forget to leave picocli a ⭐ star on GitHub if you like the project. 😉

@remkop
Copy link
Owner

remkop commented Jan 13, 2021

Please take a look at the updated section for Single Parameter Type Converters in the user manual. This should clarify the converter attribute, what do you think?

The section on default values already seems pretty clear to me:

It is possible to define a default value for an option or positional parameter, that is assigned when the user did not specify this option or positional parameter on the command line.

For an annotated setter method, "assigned" cannot mean anything else than that the setter method is called. Not sure if this can be phrased better.

Can you suggest any concrete improvements?

@asarkar
Copy link
Author

asarkar commented Jan 13, 2021

The converter docs seem fine, I just couldn’t find it until you pointed it out.
The default value, on hindsight, is understandable but it could be clearer, something like the following:
“If you’ve a setter method , and a default value declared in the annotation, the setter is called with that value. The setter is not called if no default has been declared and the user doesn’t specify that option.”

@remkop
Copy link
Owner

remkop commented Jan 16, 2021

Hi @asarkar sorry for the late reply.

I finally got around to updating the text for the Default Values section in the user manual.
Please take a look and see if this is what you had in mind.

Thanks again for the feedback, and don't forget to leave picocli a star ⭐ on GitHub to help promote the project. 😉

@remkop remkop closed this as completed Jan 16, 2021
MarkoMackic pushed a commit to MarkoMackic/picocli that referenced this issue Oct 17, 2021
MarkoMackic pushed a commit to MarkoMackic/picocli that referenced this issue Oct 17, 2021
MarkoMackic added a commit to MarkoMackic/picocli that referenced this issue Oct 17, 2021
MarkoMackic added a commit to MarkoMackic/picocli that referenced this issue Oct 17, 2021
MarkoMackic added a commit to MarkoMackic/picocli that referenced this issue Oct 17, 2021
MarkoMackic added a commit to MarkoMackic/picocli that referenced this issue Oct 17, 2021
MarkoMackic added a commit to MarkoMackic/picocli that referenced this issue Oct 17, 2021
MarkoMackic added a commit to MarkoMackic/picocli that referenced this issue Oct 17, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants