Fix UsageFormatter: inherit usage formatter for subcommands #423
Conversation
1. Remove field `commander` from DefaultUsageFormatter & UnixStyleUsageFormatter and adjust IUsageFormatter and its implementations. 2. Modify JCommander#setUsageFormatter to set usage formatter for all subcommands. 3. Instantiate subcommand with parent's usage formatter. 4. Add @OverRide annotations on methods inherited from interface or super class. 5. Modify accessibilty of some methods: not every method needs to be public. 6. Revise documents. 7. Modify tests.
|
Your changes make sense to me, but CI has failed, I think this is because of Java 9? |
|
I have removed the usage of lambda(Java 8) to avoid compilation failure. Now CI has passed. |
|
You should also update the online docs by modifying doc/index.adoc section 23. |
|
+1 any change this could be merged and a new release cut? |
|
Any updates? Should I change something to get this merged? |
|
@cbeust Please take a look when you can. |
|
Is there any chance that this PR will be accepted soon? |
|
Apologies for not acting on this PR but each time I pull it to review it, its size forces me to reschedule it for when I have more time ahead of me... |
|
@cbeust Sorry for the delayed response. The core change is modifying the interface /**
* Display the usage for this command.
*/
- void usage(String commandName);
+ void usage(JCommander commander, String commandName);And the other changes are just simple injecting this new parameter and make things work. Additionally, 5 out of 9 changed files are tests 😂. |
|
@cbeust I don't mind reviewing it, if that is cool with you. |
|
@PureCS Of course, the more pairs of eyes, the better! Thanks! |
| @@ -61,7 +61,7 @@ public void testOrder(Object cmd, String ... expected) { | |||
| JCommander commander = new JCommander(cmd); | |||
|
|
|||
| StringBuilder out = new StringBuilder(); | |||
| commander.getUsageFormatter().usage(out); | |||
| commander.usage(out); | |||
There was a problem hiding this comment.
It is, however it does read nicer.
| @@ -989,7 +989,7 @@ class ParameterNamesUsageFormatter implements IUsageFormatter { | |||
| // Extend other required methods as seen in DefaultUsageFormatter | |||
|
|
|||
| // This is the method which does the actual output formatting | |||
| public void usage(StringBuilder out, String indent) { | |||
| public void usage(JCommander commander, StringBuilder out, String indent) { | |||
There was a problem hiding this comment.
This is a breaking change (and there might be more in this PR, haven't fully reviewed yet).
Not a deal breaker, but if this gets merged, the major version will have to go up.
There was a problem hiding this comment.
@cbeust I have started a v2 branch to collect things that could go into JCommander 2.0.0. Did you find the time to finish your review, or should I start a review from scratch?
I was so happy when I found out that JCommander has supported unix-style usage formatting. But when I use it in my project, I am confused about the design of the interface.
IMO, UsageFormatter should be stateless and should never has a constructor binding to a commander.
Moreover, usage formatter is not inherited by subcommands. So if I set the formatter of root commander to unix-like,
usage()will output likeSo I changed almost all methods related to formatter to fix this.
commanderfrom DefaultUsageFormatter & UnixStyleUsageFormatterand adjust IUsageFormatter and its implementations.
subcommands.
super class.
public.
All tests are passed.