-
Notifications
You must be signed in to change notification settings - Fork 418
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
Add support for IDefaultValueProvider #321
Comments
API-wise, I think the below is sufficient for the first cut:
No need to have an |
If you can help me, I have difficulties to see how to retrieve the CommandSpec (to get my default value provider) from within the ArgSpec defaultValue() method |
I was thinking to leave |
Ok, something like that ?
|
Yes exactly! |
Also do you have an idea about where to put the test ? I was thinking about something around
|
I'm going to propose my PR without test for now for you to see if it's ok, but of course I'm going to add these test. Anyway you wont merge it if there's no tests ;) |
Following what was done for Command version provider I added a default value provider as proposed in remkop#321.
That was quick! Thanks! |
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
Agree fort the new test class. |
Thanks for the contribution! I guess the last thing is a mention of this new mechanism in the Default Values section of the user manual. I can take care of that unless you get to it first. |
Will try. |
Ok, then I'll wait for you on the docs. |
…ultValueProvider #321 add support for default value provider
I merged the PR. Many thanks! There are some other tickets I'm already working on so it may take me a bit before I will get to the user manual. If you feel like doing another PR for the user manual, go for it! :-) |
I will have a look at the doc part, I promise. Meanwhile I'm going to use the system to be sure it works well ;) |
I would need to add something to the PR I did, a boolean to indicate if the defaultprovider should override default values or not. I will propose another PR for that and the doc of course. |
What is the use case for this boolean? If someone wants a default value in the code to “win”, they can simply omit it from the config file, no? I would prefer to keep it simple. We can always enhance later if really necessary. |
Alternatively, |
Yes sometimes, when you are head down in the code you don't see obvious things... |
No worries. :-) |
Looking deeper, it seems that it however requires a fix in my previous PR. Because if I have a default or initial value, the default provider value can't apply. So that's why I wanted to go in this twisted addition I guess. But the right way to do is to fix it instead of adding more logic... |
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.
I went ahead and added some text to the user manual and release notes. Feel free to suggest further improvements. Again thanks for the contribution! |
Sorry for the delay for doc. I will review what you wrote but next time I will do more. Just that work always comes first... |
No worries! |
I just realized we forgot about |
I've given it some thought and I am leaning towards changing That meets the first goal (description text containing ${DEFAULT-VALUE} should be rendered with the default value), and at the same time allows application authors to distinguish between the default value that was programmatically set on an option (can be obtained with (Also need to update the Javadoc for these ArgSpec methods to clarify where the values come from.) |
Add support for
IDefaultValueProvider
on@Command
and potentially on individual@Option
and@Parameters
.See discussion under #261.
Proposed API:
The text was updated successfully, but these errors were encountered: