-
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
[#530] Usage message customization (2) #567
Conversation
Did not build. Shame on me. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall, thank you very much for stepping up! I've put some comments on places where I think we can improve things. Please take a look.
Minor detail: Can I ask you a favor and use spaces instead of tabs?
About Java 5 vs Java 8 lambdas: in the discussion on #530 I used lambdas for illustration purposes in a comment but in the picocli source we will need to use anonymous classes to implement the ISectionRenderer
interface. I would like picocli to only require Java 5.
@@ -1467,6 +1468,22 @@ public static void usage(Object command, PrintStream out, Help.ColorScheme color | |||
* @since 3.0 */ | |||
public void usage(PrintWriter writer, Help.Ansi ansi) { usage(writer, Help.defaultColorScheme(ansi)); } | |||
|
|||
public void setHelpFactory(IHelpFactory helpFactory) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would like to return this
CommandLine object from setHelpFactory
to be consistent with the other setter methods in CommandLine
.
this.helpFactory = helpFactory; | ||
} | ||
|
||
public IHelpFactory helpFactory() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would like to rename this method to getHelpFactory
to be consistent with the other getter methods in CommandLine
.
return helpFactory; | ||
} | ||
|
||
public CommandLine helpFactory(IHelpFactory helpFactory) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This method essentially does the same as the setHelpFactory
method. If we return this
from setHelpFactory
we can remove the helpFactory(IHelpFactory)
method.
@@ -3214,6 +3220,18 @@ public CommandLine setAtFileCommentChar(Character atFileCommentChar) { | |||
public String defaultValue(ArgSpec argSpec) { throw new UnsupportedOperationException(); } | |||
} | |||
|
|||
public interface IHelpFactory { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. We need to think of some javadoc, but I can add that later.
@@ -7925,6 +7943,11 @@ public void init(CommandLine helpCommandLine, Help.Ansi ansi, PrintStream out, P | |||
void init(CommandLine helpCommandLine, Help.Ansi ansi, PrintStream out, PrintStream err); | |||
} | |||
|
|||
|
|||
public interface IHelpSectionRenderer { | |||
String render(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was thinking to make the method signature either
/**
* Renders a section of the usage help, like header heading, header, synopsis heading,
* synopsis, description heading, description, etc.
* @since 3.9
*/
String render(CommandSpec spec, ColorScheme scheme);
or
// TODO make Help.commandSpec() method public if we choose this signature.
String render(Help help);
I think it will be convenient for implementors of this interface to be given access to the CommandSpec
and ColorScheme
for which it needs to render something.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After thinking some further the 2nd signature looks better. What do you think?
// TODO make Help.commandSpec() method public if we choose this signature.
String render(Help help);
.append(help.footer()); | ||
return help.sections().stream() | ||
.map(it -> it.render()) | ||
.collect(() -> sb, StringBuilder::append, StringBuilder::append); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here's where it becomes a bit tricky... We want users to be able to:
- reorder existing sections
- replace a default section renderer with a custom renderer
- add custom sections in between existing sections
- remove existing sections
I was thinking to do something like this:
private static StringBuilder usage(StringBuilder sb, Help help) {
for (String key : getSectionKeys()) {
ISectionRenderer renderer = sectionMap.get(key);
if (renderer != null) { sb.append(renderer.render(help); }
}
return sb;
}
// TODO we should probably provide constants for the default section keys
private List<String> sectionKeys = Collections.unmodifiableList(Arrays.asList("headerHeading",
"header",
"synopsisHeading",
"synopsis",
"descriptionHeading",
"description",
"parameterListHeading",
"parameterList",
"optionListHeading",
"optionList",
"commandListHeading",
"commandList",
"footerHeading",
"footer"));
/**
* Returns the section keys in the order that the usage help message should render the sections.
* This ordering may be modified with {@link #setSectionKeys(List) setSectionKeys}. The default keys are:
* <pre>
* "headerHeading",
* "header",
* "synopsisHeading",
* "synopsis",
* "descriptionHeading",
* "description",
* "parameterListHeading",
* "parameterList",
* "optionListHeading",
* "optionList",
* "commandListHeading",
* "commandList",
* "footerHeading",
* "footer"
* </pre>
* @since 3.9
*/
public List<String> getSectionKeys() { return sectionKeys; }
/**
* Sets the section keys in the order that the usage help message should render the sections.
* @see #getSectionKeys
* @since 3.9
*/
public void setSectionKeys(List<String> keys) { sectionKeys = Collections.unmodifiableList(keys); }
Yes, I think it may make sense to open a separate ticket for that. Essentially you want the detailedSynopsis method separated into the logic that generates the command name and the logic that generates the option and positional parameter details, correct? Once we split detailedSynopsis, the work for this ticket will certainly enable customizing the synopsis easily, I don't think we're very far away. Will you be able to add some unit tests? |
Do you think you will be able to work on this pull request further? I can do a separate 3.9 release for this if we can polish this a bit. Otherwise it will have to wait until 4.0 or later. |
Codecov Report
@@ Coverage Diff @@
## master #567 +/- ##
============================================
+ Coverage 89.14% 89.15% +<.01%
- Complexity 286 294 +8
============================================
Files 4 4
Lines 3971 3993 +22
Branches 978 980 +2
============================================
+ Hits 3540 3560 +20
- Misses 212 213 +1
- Partials 219 220 +1
Continue to review full report at Codecov.
|
@@ -8010,7 +8145,8 @@ public Help(CommandSpec commandSpec, ColorScheme colorScheme) { | |||
this.aliases.add(0, commandSpec.name()); | |||
this.colorScheme = Assert.notNull(colorScheme, "colorScheme").applySystemProperties(); | |||
parameterLabelRenderer = createDefaultParamLabelRenderer(); // uses help separator | |||
|
|||
this.helpFactory = commandSpec.commandLine() != null ? commandSpec.commandLine().getHelpFactory() : new DefaultHelpFactory(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure if this is the right way to get the helpfactory.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that looks fine. You raise a good point though: the CommandLine
class is mostly a facade, and it may be good to move some of this down to the CommandSpec
class. I will go ahead with the merge first, and I may make some changes afterwards.
Merged. Thank you for the pull request! I may make some further changes. |
First, thank you for your excellent support.
Yes, maybe today or tomorrow. |
Issue #530
Changes based on stechios changes and your suggestions.
Does this comply with your suggestions? Just say what you think.
First ever pull request, so I don't know what I am doing github-wise.
I assume that every new Help(...) inside of Help needs to be replaced by the factory?
For me all of this usage customization only has a value when Help.detailedSynopsis is split into multiple methods. This affects synopsis Help.synopsis(int). So I suggest making the switch between custom, abbreviated and detailed synopsis in Help.sections. Plus some code reuse to implement Help.synopsis(int) if the interface should not break. I think it fits this issue, or should i open a new issue for that?
My agenda: I like to see some parameters in usage instead of commandSpec.qualifiedName() and want to replace qualifiedName() with a subcommand-loop. e.g. we do 'thingy 119 update foo=13' and expect as subcommand help synopsis: thingy update.