Skip to content

Fix UsageFormatter: inherit usage formatter for subcommands#423

Open
arkbriar wants to merge 3 commits into
cbeust:masterfrom
arkbriar:master
Open

Fix UsageFormatter: inherit usage formatter for subcommands#423
arkbriar wants to merge 3 commits into
cbeust:masterfrom
arkbriar:master

Conversation

@arkbriar

Copy link
Copy Markdown

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.

JCommander commander = JCommander.newBuilder()
    .setUsageFormatter(new UnixStyleUsageFormatter(/*What should I set here???*/))
    .addObject(options).build();

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 like

Usage: <main class> [options] [command] [command options]
  Options:
    [unix style]
  Commands:
    [default style]

So I changed almost all methods related to formatter to fix this.

  • Remove field commander from DefaultUsageFormatter & UnixStyleUsageFormatter
    and adjust IUsageFormatter and its implementations.
  • Modify JCommander#setUsageFormatter to set usage formatter for all
    subcommands.
  • Instantiate subcommand with parent's usage formatter.
  • Add @OverRide annotations on methods inherited from interface or
    super class.
  • Modify accessibilty of some methods: not every method needs to be
    public.
  • Revise documents.
  • Modify tests.

All tests are passed.

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.
@dozmus

dozmus commented Apr 9, 2018

Copy link
Copy Markdown
Contributor

Your changes make sense to me, but CI has failed, I think this is because of Java 9?

@arkbriar

Copy link
Copy Markdown
Author

I have removed the usage of lambda(Java 8) to avoid compilation failure. Now CI has passed.

@dozmus

dozmus commented Apr 11, 2018

Copy link
Copy Markdown
Contributor

You should also update the online docs by modifying doc/index.adoc section 23.

@jgangemi

jgangemi commented May 9, 2018

Copy link
Copy Markdown

+1

any change this could be merged and a new release cut?

@arkbriar

Copy link
Copy Markdown
Author

Any updates? Should I change something to get this merged?

@dozmus

dozmus commented Jun 25, 2018

Copy link
Copy Markdown
Contributor

@cbeust Please take a look when you can.

@PoslavskySV

Copy link
Copy Markdown

Is there any chance that this PR will be accepted soon?

@cbeust

cbeust commented Jul 11, 2018

Copy link
Copy Markdown
Owner

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...

@arkbriar

Copy link
Copy Markdown
Author

@cbeust Sorry for the delayed response. The core change is modifying the interface IUsageFormatter by adding a new JCommander argument, which stands for the commander who request the format. For example,

    /**
     * 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 😂.
Looking forward to hearing from you. If any effort I should make to get this merged, please let me know.

@dozmus

dozmus commented Sep 20, 2018

Copy link
Copy Markdown
Contributor

@cbeust I don't mind reviewing it, if that is cool with you.

@cbeust

cbeust commented Sep 21, 2018

Copy link
Copy Markdown
Owner

@PureCS Of course, the more pairs of eyes, the better!

Thanks!


StringBuilder out = new StringBuilder();
commander.getUsageFormatter().usage(out);
commander.usage(out);

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't this change a step back?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is, however it does read nicer.

Comment thread doc/index.adoc

// 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) {

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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?

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's start from scratch.

@mkarg mkarg force-pushed the master branch 2 times, most recently from 4dbceb5 to a1d8505 Compare October 5, 2025 13:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants