Copilot commented on code in PR #4443:
URL: https://github.com/apache/solr/pull/4443#discussion_r3259354939
##########
solr/core/src/java/org/apache/solr/cli/ConfigSetOptions.java:
##########
@@ -28,7 +28,16 @@ public class ConfigSetOptions {
@CommandLine.Option(
names = {"-n", "--conf-name"},
- description = "Configset name in ZooKeeper.",
+ description = {
+ "Configset name in ZooKeeper.",
+ "Name of the configuration set in ZooKeeper. This command will upload
the configuration set to the \"configs\" ZooKeeper node giving it the name
specified.",
+ "",
+ "You can see all uploaded configuration sets in the Admin UI via the
Cloud screens. Choose Cloud → Tree → configs to see them.",
+ "",
+ "If a pre-existing configuration set is specified, it will be
overwritten in ZooKeeper.",
+ "",
Review Comment:
The new multi-line `conf-name` description is shared by both `zk upconfig`
and `zk downconfig` via this mixin, but it currently says the command will
*upload* and that an existing configset will be overwritten in ZooKeeper. That
wording is incorrect/misleading for `downconfig` (which downloads to the local
filesystem) and the overwrite behavior isn’t an attribute of the `conf-name`
option itself. Consider making this description command-neutral (e.g., just
explain it’s the configset name under `/configs`) or splitting into separate
option definitions per command so each can describe its own overwrite semantics
accurately.
##########
solr/solr-ref-guide/modules/deployment-guide/pages/cli/solr-zk-downconfig.adoc:
##########
@@ -54,6 +54,20 @@ Download a configset from ZooKeeper to the local filesystem.
*-n*, *--conf-name*=_<confName>_::
Configset name in ZooKeeper.
++
+Name of the configuration set in ZooKeeper. This command will upload the
configuration set to the "configs" ZooKeeper node giving it the name specified.
++
+
++
+You can see all uploaded configuration sets in the Admin UI via the Cloud
screens. Choose Cloud → Tree → configs to see them.
++
+
++
+If a pre-existing configuration set is specified, it will be overwritten in
ZooKeeper.
++
+
++
Review Comment:
This generated ref-guide text for `zk downconfig` says the command will
*upload* the configset and that existing configsets will be overwritten in
ZooKeeper, which contradicts the command’s purpose (“Download a configset from
ZooKeeper to the local filesystem”). The source annotation for this option
description should be corrected so the generated docs match the actual
`downconfig` behavior.
##########
solr/core/src/java/org/apache/solr/cli/SolrCLI.java:
##########
@@ -109,8 +109,33 @@ public static void main(String[] args) throws Exception {
}
}
+ /**
+ * Truncates each option's description to its first line for interactive
{@code --help} output.
+ * The remaining lines of the {@code description} array are reserved for the
generated ref-guide.
+ * ManPageGenerator bypasses this customization (it always calls {@code
+ * createDefaultOptionRenderer()} on a fresh Help), so the .adoc still shows
the full description.
+ */
+ private static void installFirstLineOnlyHelpFactory(picocli.CommandLine cmd)
{
+ cmd.setHelpFactory(
+ (spec, colorScheme) ->
+ new picocli.CommandLine.Help(spec, colorScheme) {
+ @Override
+ public picocli.CommandLine.Help.IOptionRenderer
createDefaultOptionRenderer() {
+ picocli.CommandLine.Help.IOptionRenderer base =
super.createDefaultOptionRenderer();
+ return (option, paramLabelRenderer, scheme) -> {
+ picocli.CommandLine.Help.Ansi.Text[][] rows =
+ base.render(option, paramLabelRenderer, scheme);
+ return rows.length <= 1
+ ? rows
+ : new picocli.CommandLine.Help.Ansi.Text[][] {rows[0]};
Review Comment:
The renderer truncation is applied *after* picocli has wrapped the option
help into table rows, so this effectively keeps only the first *wrapped row*,
not the first *description line*. If the first description line exceeds the
configured help width, the CLI help will show an incomplete, width-truncated
fragment. Consider truncating based on the raw description (e.g., only use
`option.description()[0]`) and then let picocli wrap that single line, instead
of dropping wrapped rows.
##########
solr/core/src/java/org/apache/solr/cli/SolrCLI.java:
##########
@@ -109,8 +109,33 @@ public static void main(String[] args) throws Exception {
}
}
+ /**
+ * Truncates each option's description to its first line for interactive
{@code --help} output.
+ * The remaining lines of the {@code description} array are reserved for the
generated ref-guide.
+ * ManPageGenerator bypasses this customization (it always calls {@code
+ * createDefaultOptionRenderer()} on a fresh Help), so the .adoc still shows
the full description.
Review Comment:
The Javadoc claims ManPageGenerator “bypasses this customization” because it
calls `createDefaultOptionRenderer()` on a fresh `Help`, but the ref-guide
generation runs `picocli.codegen.docgen.manpage.ManPageGenerator` directly (see
`solr-ref-guide/build.gradle`) and does not execute `SolrCLI.main()` /
`propagateCommandSettings()`. Consider rewording this comment to reflect the
actual reason the doc generation output is unaffected (i.e., the custom help
factory is only installed for interactive CLI execution).
##########
solr/core/src/java/org/apache/solr/cli/SolrCLI.java:
##########
@@ -109,8 +109,33 @@ public static void main(String[] args) throws Exception {
}
}
+ /**
+ * Truncates each option's description to its first line for interactive
{@code --help} output.
+ * The remaining lines of the {@code description} array are reserved for the
generated ref-guide.
+ * ManPageGenerator bypasses this customization (it always calls {@code
+ * createDefaultOptionRenderer()} on a fresh Help), so the .adoc still shows
the full description.
+ */
+ private static void installFirstLineOnlyHelpFactory(picocli.CommandLine cmd)
{
+ cmd.setHelpFactory(
+ (spec, colorScheme) ->
+ new picocli.CommandLine.Help(spec, colorScheme) {
+ @Override
+ public picocli.CommandLine.Help.IOptionRenderer
createDefaultOptionRenderer() {
+ picocli.CommandLine.Help.IOptionRenderer base =
super.createDefaultOptionRenderer();
+ return (option, paramLabelRenderer, scheme) -> {
+ picocli.CommandLine.Help.Ansi.Text[][] rows =
+ base.render(option, paramLabelRenderer, scheme);
+ return rows.length <= 1
+ ? rows
+ : new picocli.CommandLine.Help.Ansi.Text[][] {rows[0]};
+ };
+ }
+ });
+ }
Review Comment:
This behavior change (custom help factory/option renderer) isn’t covered by
existing CLI tests. Adding a focused unit test that asserts `--help` output
shows only the first description line (and that long first lines still wrap
correctly) would help prevent regressions when upgrading picocli or adjusting
help widths.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
