-
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
Excludes hidden options, params, and subcommands from man page generation #1064
Conversation
@@ -6693,6 +6705,7 @@ void initDefaultValueProvider(Class<? extends IDefaultValueProvider> value, IFac | |||
void updateHelpCommand(boolean value) { if (isNonDefault(value, DEFAULT_IS_HELP_COMMAND)) {isHelpCommand = value;} } | |||
void updateSubcommandsRepeatable(boolean value) { if (isNonDefault(value, DEFAULT_SUBCOMMANDS_REPEATABLE)) {subcommandsRepeatable = value;} } | |||
void updateAddMethodSubcommands(boolean value) { if (isNonDefault(value, DEFAULT_IS_ADD_METHOD_SUBCOMMANDS)) {isAddMethodSubcommands = value;} } | |||
void updateHidden(boolean value) { if (isNonDefault(value, DEFAULT_HIDDEN)) { isHidden = value; } } |
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.
@remkop I didn't see another way of access the hidden
prop, but I'm probably missing something as it is used for the help text output.
If you have any alternative idea let me know, and I could remove this method (and revert the MINOR version bump)
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.
@bdemers The @Command(hidden)
value is captured as a property of the command's UsageHelpMessageSpec
. I admit this is not easy to find. Perhaps the javadoc for picocli.CommandLine.Command#hidden
should have a @see picocli.CommandLine.Model.UsageMessageSpec#hidden()
reference to make this a bit easier to find. Can you do that as part of the PR?
The Help
class has its own map of subcommands, and when this map is built up in CommandLine.Help#addAllSubcommands
, hidden subcommands are excluded. This means that the logic for Help
is simpler, because it does not need to care about hidden subcommands any more, but can be a bit puzzling if you miss this. This should be mentioned in the javadoc for Help#addAllSubcommands
and the javadoc for Help#subcommands
. Can you include that in the PR?
Good finds, thanks! 👍
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.
Since we can use picocli.CommandLine.Model.UsageMessageSpec#hidden
, all changes to CommandLine
can be reverted.
Codecov Report
@@ Coverage Diff @@
## master #1064 +/- ##
============================================
+ Coverage 94.35% 94.46% +0.10%
- Complexity 442 448 +6
============================================
Files 2 2
Lines 6484 6571 +87
Branches 1736 1765 +29
============================================
+ Hits 6118 6207 +89
+ Misses 94 93 -1
+ Partials 272 271 -1
Continue to review full report at Codecov.
|
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 good progress! Thanks for taking care of this!
I added some more comments.
gradle.properties
Outdated
@@ -16,7 +16,7 @@ springBootVersion = 2.2.2.RELEASE | |||
projectPreviousReleaseVersion = 4\\.3\\.2 | |||
# projectPreviousVersionRegex may be a SNAPSHOT | |||
projectPreviousVersionRegex = 4\\.3\\.2 | |||
projectVersion = 4.3.3-SNAPSHOT | |||
projectVersion = 4.4.0-SNAPSHOT |
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 think this can be reverted.
@@ -193,7 +193,7 @@ static int generateManPage(Config config, CommandSpec... specs) throws IOExcepti | |||
// recursively create man pages for subcommands | |||
for (CommandLine sub : spec.subcommands().values()) { | |||
CommandSpec subSpec = sub.getCommandSpec(); | |||
if (done.contains(subSpec)) {continue;} | |||
if (done.contains(subSpec) || subSpec.hidden()) {continue;} |
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.
We can use picocli.CommandLine.Model.UsageMessageSpec#hidden
here.
pw.printf(" %s%n", rows[0][4]); | ||
for (int i = 1; i < rows.length; i++) { | ||
pw.printf("+%n%s%n", rows[i][4]); | ||
if (!positional.hidden()) { |
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.
Should we have a similar if statement in writeOption
: if (!option.hidden()) ...
,
or should we exclude hidden PositionalParamSpec
before the writePositional
method is called?
I would like it if we can do this in a consistent manner for options, positionals and commands.
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.
+1, keeping the check in writeOption
/writePositional
is easier, as writePositional
is called from multiple places.
However, the check for the subcommands gets a little trickier, the subcommands need to be removed before processing to avoid the tag/"Commands" heading.
Thoughts?
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 about this some more, I think your original approach to filter out hidden args first is better.
The reason is the same as for subcommands: if all options or positional parameters are hidden, we don't want to include the tag and heading in the output. (see also #1071)
@@ -6693,6 +6705,7 @@ void initDefaultValueProvider(Class<? extends IDefaultValueProvider> value, IFac | |||
void updateHelpCommand(boolean value) { if (isNonDefault(value, DEFAULT_IS_HELP_COMMAND)) {isHelpCommand = value;} } | |||
void updateSubcommandsRepeatable(boolean value) { if (isNonDefault(value, DEFAULT_SUBCOMMANDS_REPEATABLE)) {subcommandsRepeatable = value;} } | |||
void updateAddMethodSubcommands(boolean value) { if (isNonDefault(value, DEFAULT_IS_ADD_METHOD_SUBCOMMANDS)) {isAddMethodSubcommands = value;} } | |||
void updateHidden(boolean value) { if (isNonDefault(value, DEFAULT_HIDDEN)) { isHidden = value; } } |
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.
Since we can use picocli.CommandLine.Model.UsageMessageSpec#hidden
, all changes to CommandLine
can be reverted.
FYI: This issue may be relevant: #1071 |
90060d0
to
2f6de1c
Compare
Still one more comment to address, but I reverted the new method and version bump |
dc93d5e
to
245ef7f
Compare
Thanks for the updates! Please let me know when this is ready for review. |
Hey @remkop it's ready. though I did have a question/comment: https://github.com/remkop/picocli/pull/1064/files#r430546512 |
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 replied to your comment, but #1071 made clear that your original approach is better:
Filter out hidden args first allows us to avoid emitting a header without any content in the man page.
@remkop good call, I updated the test to make sure the Arguments header is not included when all of the args are hidden |
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 think we are very close now.
For positional parameters, the man page now correctly does not show an == Arguments
header if all positionals are either hidden, or part of a group (so it will be shown under the group header).
We should do the same for the == Options
header.
if (iter.next().hidden()) { | ||
iter.remove(); | ||
} | ||
} |
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 believe this is the right approach, but we should do this at the top of this method, before rendering the == Options
header.
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.
Ahh, I missed that one, i just pushed an update
It may be good to have a test for the case where all options are hidden. |
I went ahead and merged this into master. |
Fixes: #1063