1409 documentation tests #1463
1409 documentation tests #1463
Conversation
|
Thank you for working on this! This PR modifies two files:
The changes to the unit test look fine. However, the index.html file is generated from the Asciidoc |
… in Unreferenced Argument Groups"
|
@remkop Well there, you do not know what you do not know. I did the html formatting by hand. The .adoc was much simpler. Thank you for being patient with me through this process. |
|
I see your comments (like Edit made in branch.), but I don't see your changes. Did you push? |
|
I did not push yet. I am working on some documentation and the kotlin changes. For kotlin, unlike java you must declare a type up front. I am getting held up on what the run function should look like and it appears unnecessary for kotlin, unlike java. I am not a kotlin expert but to me it seems that in java you can declare a variable type without instantiation it |
|
I have a section written for kotlin I can push, but I do not believe it is technically correct: |
|
I tested the sample code and it does not work. I am going to modify the index files appropriately: |
|
What is the error? |
Cannot read field "one" because "obj.optXAndGroupOneOrGroupTwo.oneORtwo" is null |
|
Before asserting the test results, call the |
|
When I change it to call execute before parseArgs I still get an error about NullPointerException. However, specifying that the user calls the execute command could lead to unintended results. Typically, if the user performs their command, the intention is that it is ran one time and under the stipulated arguments provided. If the command were to delete files within a folder, except those specified by arguments or with certain attributes we could unintentionally direct the user to run the command twice, once without arguments to initialize the Argroup and again once the arguments have been parsed. I propose that we demonstrate this to the user through declaring the ArgGroups while instantiating them, as shown: Here is the error when I call execute() before parseArgs(): |
|
Sorry I was unclear. I meant "please call the @Test
public void testIssue1409() {
final Issue1409Mod obj = new Issue1409Mod();
new CommandLine(obj).execute("-x", "ANOTHER_VALUE"); // <--- change parseArgs to execute
assertEquals("Default value for X incorrect","ANOTHER_VALUE", obj.optXAndGroupOneOrGroupTwo.x);
assertEquals("Default value for _1a incorrect",null, obj.optXAndGroupOneOrGroupTwo.oneORtwo.one._1a);
assertEquals("Default value for _1b incorrect",null, obj.optXAndGroupOneOrGroupTwo.oneORtwo.one._1b);
assertEquals("Default value for _2a incorrect","Default 2A", obj.optXAndGroupOneOrGroupTwo.oneORtwo.two._2a);
assertEquals("Default value for _2b incorrect","Default 2B", obj.optXAndGroupOneOrGroupTwo.oneORtwo.two._2b);
} |
|
I'll do that now. One moment. |
|
You're a genius...i'll make the changes and re-commit. I had to change to class implements runnable. |
|
In our new section in the user manual, I would like to recommend that applications do the following:
public void run() {
if (myGroup == null) {
// perform any logic that needs to happen if none of the
// options in the group were specified on the command line
myGroup = new MyGroup(); // initialize the group and set default values for the options
}
// remaining business logic that uses the default values
} |
|
I'll make that change in the .adoc and .html files. |
…classes instantiated declaratively and one where we initialize the values in execution.
…classes instantiated declaratively and one where we initialize the values in execution.
|
I will work on the documentation files next. |
Don't worry about the html file, I will regenerate it anyway. |
Index.adoc - Changed entire section to reflect guidance from PR.
|
@remkop I rewrote the section. The original perspective was discussing what needed to be changed to handle 1409 specific problems. The new section is more ambiguous, and instead of addressing why things need to be done in a specific way, simply states these are the guidelines for successful implementation. I also added the portion about how we instantiate objects. |
| ---- | ||
| @Option(names= {"-x"}, defaultValue = "X") String X = "X"; | ||
| ---- | ||
| Matching the annotation and declaration ensures that picocli sets the default values regardless of the user supplies the arguments. |
There was a problem hiding this comment.
I think it will be good to acknowledge that there is some duplication here, just to be clear and explicit that this is intentional.
Also (nitpicking again), it is actually not picocli who is setting the default value here: remember that the group is not instantiated when there is no match. By setting an initial value, the application is actually doing the work of assigning a default value themselves. I think it is good to be explicit that this is necessary, and mention something like the below, somewhere in this section:
When it comes to assigning default values to options in argument groups, applications need to do extra work that is not necessary with options outside of argument groups.
|
|
||
| Next, identify your preferred default behavior. For `+@ArgGroup+`'s there are two distinct ways of instantiating an object. | ||
|
|
||
| The first is to instantiate the object declaratively. This behavior is best when you desire no ambiguity in whether or not an `+@ArgGroup+` is instantiated. |
There was a problem hiding this comment.
This behavior is best when you desire no ambiguity in whether or not an
+@ArgGroup+is instantiated.
I imagine some readers may be confused as to what ambiguity you are referring to. How about something like this?
The first is to instantiate the object in the declaration. This way, the
@ArgGroup-annotated field is nevernull, even if none of the options in that group are specified on the command line.
|
I went ahead and merged it in. |
|
I had been consumed by work. I should have said i was going to clean it up on wednesday. |
No problem at all! Thanks for thinking through this together. This is quite a complex topic and our discussion helped clarify and flesh out the new section. Thank you! |
Updated based on guidance from @remkop