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

How to programmatically set a value when an @Option wasn't specified to the console? #1915

Closed
DavidTheExplorer opened this issue Jan 6, 2023 · 21 comments

Comments

@DavidTheExplorer
Copy link

After I couldn't find an answer on the docs, I'm opening this issue.
I have been looking for a programmatical alternative for defaultValue, and after trial and error I discovered this is possible:

@Option(names = "-age")
private int age = AgeUtils.inferUserAge();

public int getAge()
{
    return this.age;
}

Which works, but AgeUtils.inferUserAge is called even if the user's age was supplied - so I assume it's an incorrect use of the API.

@remkop
Copy link
Owner

remkop commented Jan 6, 2023

Hi David!
I see your comment, and I can maybe guess, but let me just ask:
What is your question?

@DavidTheExplorer
Copy link
Author

DavidTheExplorer commented Jan 6, 2023

Hi David! I see your comment, and I can maybe guess, but let me just ask: What is your question?
Am I using picoli correctly? is there a way to call AgeUtils.inferUserAge() only if the option isn't specified?

@DavidTheExplorer DavidTheExplorer changed the title How to properly set a programmatically obtained value when a @Option field wasn't specified? How to programmatically set a value when a @Option field wasn't specified? Jan 6, 2023
@DavidTheExplorer DavidTheExplorer changed the title How to programmatically set a value when a @Option field wasn't specified? How to programmatically set a value when an @Option wasn't specified? Jan 6, 2023
@DavidTheExplorer DavidTheExplorer changed the title How to programmatically set a value when an @Option wasn't specified? How to programmatically set a value when an @Option wasn't specified to the console? Jan 6, 2023
@remkop
Copy link
Owner

remkop commented Jan 6, 2023

The initial value is assigned to the field by the Java runtime, not by picocli.

The alternative way to assign default values is via the annotation API , and that won’t work either because it requires compile-time constants.

So in short, no, there’s no way to tell picocli that AgeUtils.inferUserAge() is the default value and only invoke this method if the option is not specified on the command line.

You could of course leave the age field blank and in the business logic invoke AgeUtils.inferUserAge() only if age is blank but then picocli won’t know the default value so it won’t be displayed in the usage help message.

@DavidTheExplorer
Copy link
Author

The initial value is assigned to the field by the Java runtime, not by picocli.

The alternative way to assign default values is via the annotation API , and that won’t work either because it requires compile-time constants.

So in short, no, there’s no way to tell picocli that AgeUtils.inferUserAge() is the default value and only invoke this method if the option is not specified on the command line.

You could of course leave the age field blank and in the business logic invoke AgeUtils.inferUserAge() only if age is blank but then picocli won’t know the default value so it won’t be displayed in the usage help message.

So perhaps you can add such a functionality? I don't see why the IDefaultValueProvider interface can't be used here as well.

@remkop
Copy link
Owner

remkop commented Jan 7, 2023

So perhaps you can add such a functionality? I don't see why the IDefaultValueProvider interface can't be used here as well.

Good point! That should just work with the current version of picocli!
Did you try creating a IDefaultValueProvider that calls AgeUtils.inferUserAge() for the age option, and did that not work?

@remkop
Copy link
Owner

remkop commented Jan 7, 2023

@DavidTheExplorer
Copy link
Author

DavidTheExplorer commented Jan 7, 2023

So perhaps you can add such a functionality? I don't see why the IDefaultValueProvider interface can't be used here as well.

Good point! That should just work with the current version of picocli! Did you try creating a IDefaultValueProvider that calls AgeUtils.inferUserAge() for the age option, and did that not work?

Thank you, but I think we have a misunderstanding.
My point was thst Option doesn't have a defaultProvider field, unlike Command. What I was suggesting is the possibility of adding a default provider field to Option as well

@DavidTheExplorer
Copy link
Author

DavidTheExplorer commented Jan 7, 2023

Also, another issue I just figured, does picoli support returning custom objects as default values that aren't Strings?

@remkop
Copy link
Owner

remkop commented Jan 7, 2023

So perhaps you can add such a functionality? I don't see why the IDefaultValueProvider interface can't be used here as well.

Good point! That should just work with the current version of picocli! Did you try creating a IDefaultValueProvider that calls AgeUtils.inferUserAge() for the age option, and did that not work?

Thank you, but I think we have a misunderstanding. My point was thst Option doesn't have a defaultProvider field, unlike Command. What I was suggesting is the possibility of adding a default provider field to Option as well

Oh I see what you mean now. That would require picocli to manage default provider objects for each option and positional parameter but I don't see what value it would add over the current per-command default provider.

@remkop
Copy link
Owner

remkop commented Jan 7, 2023

Also, another issue I just figured, does picoli support returning custom objects as default values that aren't Strings?

The defaultValue in the annotation and the value returned by IDefaultProvider are Strings. This String can be converted to a custom object by the type converter for that option or positional parameter.

@DavidTheExplorer
Copy link
Author

DavidTheExplorer commented Jan 7, 2023

Oh I see what you mean now. That would require picocli to manage default provider objects for each option and positional parameter but I don't see what value it would add over the current per-command default provider.

To be honest, I never touched picoli's commands because my program doesn't offer any commands.
In reality, my program is a utility for a certain game, and for my program to work I need to know the game's installation path which can be supplied by the user(File object marked as Option), Otherwise I would grab the default path from a util class.

So should I switch to working with just 1 command? That would solve my problem, but conceptually that would be wrong in my opinion.

@remkop
Copy link
Owner

remkop commented Jan 7, 2023

In my view, any CLI program or utility is a command, whether it has a @Command annotation or not. 😅

The @Command annotation is useful even if your program does not have subcommands, because it is the place where many things can be configured, like aspects of usage help message and various other things.

It is also where the command's default provider is configured so, yes, that is how I would approach your use case.

@remkop
Copy link
Owner

remkop commented Jan 7, 2023

The example application in the user manual is a simple command without subcommands and demonstrates how the @Command annotation can be useful even in that case.

@DavidTheExplorer
Copy link
Author

DavidTheExplorer commented Jan 11, 2023

The example application in the user manual is a simple command without subcommands and demonstrates how the @Command annotation can be useful even in that case.

Hey, I actually got lost... Can I only define just 1 default value provider per command that should handle every missing Option? This doesn't sound ideal, but even if I take this approach, I got completely lost with the implementation...

@Override
	public String defaultValue(ArgSpec argSpec) throws Exception 
	{
		if(argSpec.isOption() && argSpec.isValueGettable() && argSpec.paramLabel().contains("gameFolder"))
			return argSpec.getValue();
		
		else
			return GameUtils.getGameFolder().getPath();
	}

@remkop
Copy link
Owner

remkop commented Jan 11, 2023

Hey, I actually got lost... Can I only define just 1 default value provider per command that should handle every missing Option?

Yes that is correct.

if I take this approach, I got completely lost with the implementation...

I see some code you posted with that observation, but can you clarify what the problem is exactly?

@remkop
Copy link
Owner

remkop commented Jan 11, 2023

if I take this approach, I got completely lost with the implementation...

I see some code you posted with that observation, but can you clarify what the problem is exactly?

After reading the code in more detail I think I get it.
Your default provider just needs to return the default value for the argument. It does not need to worry about whether that ArgSpec already has a value or not.

I would do:

@Override
public String defaultValue(ArgSpec argSpec) throws Exception 
{
	if (argSpec.isOption() && "--gameFolder".equals(((OptionSpec) argSpec).longestName()))
		return GameUtils.getGameFolder().getPath(); // the default value for the --gameFolder option
	}
}

@remkop
Copy link
Owner

remkop commented Jan 12, 2023

This discussion made me realize that the manual does not have an implementation example for IDefaultProvider.
Thanks for raising that!
I will improve the docs.

@DavidTheExplorer
Copy link
Author

DavidTheExplorer commented Jan 13, 2023

Hello, if I understand you correctly: (by "value" I mean Parameter/Option)

  1. IDefaultValueProvider is called for every unspecified value.
  2. In my case, OptionSpec#longestName returns -gameFolder because it's the only name - and thus the longest.

If my understanding is correct, I do think a provider per value is better because:

  1. It's a modular approach.
  2. The providers become way clearer because the classes themselves only handle 1 value, and also it's easier to know
    the provider just by looking at the value's annotation.
    Currently if 2 or more values require default values, we end up having a list of if-elses which is a bad sign in
    general, unreadable, and might result in unexpected exceptions in the long run.

I really hope you can consider this ^.^

Edit: Based on the code you provided, I made this possible using this class, do you think I should PR it if you decide to reject the per-value-provider suggestion?

@remkop
Copy link
Owner

remkop commented Jan 13, 2023

I agree the provider-per-option design would be simpler/clearer for your use case, but is it really a good idea to add API to the library just for that reason? The existing provider-per-command design covers your use case, and many other use cases. Isn't that what a library's API is supposed to look like: to be generic enough to cover many use cases?

There are some use cases that do not yet have a solution, and those take priority. (For example, #1595)

Thanks for the offer to provide a PR!
In my work projects I tend to only introduce an AbstractXyz class when I see a lot of common functionality between multiple implementations and I can pull that into an abstract superclass so that all subclasses can avoid writing this logic themselves. I am not sure that this pattern applies here because I don't know how IDefaultProvider is implemented in the wild by picocli users. I have heard that some people pull values from a database, or from some config file, or even query some other application. So I am fine with just the interface without an abstract base class in picocli for now.

I did improve the examples, especially https://github.com/remkop/picocli/blob/main/picocli-examples/src/main/java/picocli/examples/defaultprovider/SimpleDefaultProviderDemo.java, hopefully that helps others get up and running quickly.

Interesting discussion, many thanks!

@DavidTheExplorer
Copy link
Author

DavidTheExplorer commented Jan 13, 2023

Alright, first of all I really appreciate the elaborate replies and your will to help me :)

I too agree that a library should provide the most generic API in order to cover all use cases, but on the other hand it's worthwhile to consider simplifications if most use cases are implemented a specific way.
The class I wrote is simply a facade for your code: It gives a utility for mapping Options to IDefaultValueProviders by the longest name, which you can also ignore and query a DB instead.

Consider an unrelated example:

public class ExceptionHandlerFactory
{
	public static ExceptionHandler logToConsole(String errorMessage) 
	{
		return exception -> System.err.println(String.format(errorMessage, exception));
	}
}

//Somewhere in your code: Fluent API + Static import magic:
new FileDownloader("www.coolphotos.com/download/abcde")
.onFinish(file -> ...)
.onError(logToConsole("Cannot download the file because of %s!"))
.download();

The factory here is a facade for the commonly used logging handler, which in reality would be complicated for different scenarios.

Back to our discussion: The class doesn't have to be prefixed with "Abstract", but I guess I will have to carry one extra class for every picoli project I will have.
It's okay if I still didn't convience you, since I got my coding answer you can close this issue 👍

@remkop
Copy link
Owner

remkop commented Jan 16, 2023

Glad you were able to get it working!

@remkop remkop closed this as completed Jan 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants