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

Consider adding PropertiesDefaultProvider implementation #868

Closed
remkop opened this issue Nov 18, 2019 · 15 comments
Closed

Consider adding PropertiesDefaultProvider implementation #868

remkop opened this issue Nov 18, 2019 · 15 comments

Comments

@remkop
Copy link
Owner

remkop commented Nov 18, 2019

Consider including the PropertiesDefaultProvider implementation of the IDefaultValueProvider interface in the picocli distribution.

@remkop remkop added this to the 4.1 milestone Nov 18, 2019
remkop added a commit that referenced this issue Nov 20, 2019
…DefaultProvider` that loads default values from properties file in home directory or specified location.
@remkop remkop closed this as completed in e976299 Nov 20, 2019
@remkop
Copy link
Owner Author

remkop commented Nov 25, 2019

@Fernal73 to follow up on our conversion about user-editable defaulting: PropertiesDefaultProvider is included in picocli 4.1, which was released a few days ago.

https://github.com/remkop/picocli/releases/tag/v4.1.0

@remkop
Copy link
Owner Author

remkop commented Nov 27, 2019

@Fernal73 mentions several other tools that provide similar functionality:

@linusjf
Copy link

linusjf commented Nov 27, 2019 via email

@remkop
Copy link
Owner Author

remkop commented Nov 27, 2019

I haven't tested this, but I believe this can be implemented in picocli with multiple passes:

  1. parse once, looking only for --no-config and --config-precedence
  2. if --no-config was specified, do not install a default provider, and parse the input again;
  3. if --config-precedence=cli-override was specified, install PropertiesDefaultProvider and parse the input again;
  4. if --config-precedence=file-override was specified, do not install a default provider, but read the config file as an @-file, add the path to this @-file to the command line arguments and parse the input again;
  5. if --config-precedence=prefer-file was specified, install PropertiesDefaultProvider and call CommandLine.execute with an empty command line arguments array.

Admittedly, point 4 may not be as simple as that... It may be necessary to inspect the ParseResult and inspect which options were actually specified on the command line, to determine whether to use the value from the command line or from the configuration.

@linusjf
Copy link

linusjf commented Nov 27, 2019

My understanding of point 4 is that the config file is still a properties file except that its values trump the CLI options. You do bring up an interesting point if you support @files simultaneously. My understanding is that it be treated as an extension of CLI options and used or discarded as per the policy option provided and if respecified (and permitted by policy) , parsed backwards so that the last value specified is the value used.
What is to be done about single-valued params in @files? If treated as command-line expansions, they shouldn't be allowed to be on the command line twice or more including @files. What about toggle parameters, though? They can be specified multiple times.

@linusjf
Copy link

linusjf commented Nov 27, 2019 via email

@remkop
Copy link
Owner Author

remkop commented Nov 27, 2019

There is an option to allow overwriting single-value options, and by default picocli does not toggle booleans when they are specified multiple times.

However, as I thought some more about this, I don't think the @-file approach is going to work. For one thing, the current PropertiesDefaultProvider expects the file to have key-value pairs like option=value, not --option=value.

That could be fixed, of course, but another (bigger) problem is multi-value options; if the user specifies a few values, and the config file also has one or more values, then the collection or map would end up with the union of both, instead of only the config file values.

It may be easier to implement --config-precedence=file-override with something like this:

  1. create a CommandLine instance, install PropertiesDefaultProvider and call CommandLine.parseArgs with an empty command line arguments array. Let's call this the config instance.
  2. create another, separate, CommandLine instance without a default provider (let's call it the userInput instance), and parse the command line input with parseArgs; this returns a ParseResult object
  3. Loop over the matched options and matched positional parameters in the ParseResult from the userInput instance that were matched on the command line, and for each of these, check if the same option or positional parameter in the config instance has a default value. If not, overwrite the value with the userInput value.
  4. Invoke the business logic on the config instance.

This would also work for argument groups, although it is more cumbersome to loop over those from the ParseResult.

@linusjf
Copy link

linusjf commented Nov 27, 2019 via email

@linusjf
Copy link

linusjf commented Nov 27, 2019 via email

@remkop
Copy link
Owner Author

remkop commented Nov 27, 2019

@Fernal73 If there is a feature you want added to picocli, it may be better to raise a new ticket so I won't forget about it. :-)

That said, in the spirit of "make common things easy and uncommon things possible", I believe it made sense to add PropertiesDefaultProvider to the library, but more complex use cases like configurations that override command-line args should probably be handled in the application.

If you have a concrete use case, I would be happy to work through it together to see how we could make it work. It might make sense to then post the resulting program as an example to picocli-examples to help other users.

@linusjf
Copy link

linusjf commented Nov 27, 2019

I'm inclined to agree with you. That was my initial reaction when I first discovered Pretty Printer as well. It still requires two passes. I , somehow, got the impression that it'd take four passes. Maybe, I wasn't thinking too clearly.
It depends on how many command-line implementations do something similar. If it's uncommon, it's best to let the application developer do it. If it's increasingly common, then it becomes expected behaviour. Then, the API might need to provide that.

@remkop
Copy link
Owner Author

remkop commented Nov 28, 2019

This topic may also be a good idea for an article or blog post.

I added Advanced Configuration to my list of ideas for articles.

@linusjf
Copy link

linusjf commented Nov 28, 2019 via email

@remkop
Copy link
Owner Author

remkop commented Nov 28, 2019

If there is a specific use case you want to investigate together, would you mind creating a separate ticket that outlines what you want to achieve? (We can decide whether this should be added to the picocli API or not once we have arrived at a solution.)

@linusjf
Copy link

linusjf commented Nov 28, 2019 via email

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