Copilot commented on code in PR #5063:
URL: https://github.com/apache/solr/pull/5063#discussion_r4224570583
##########
solr/core/src/java/org/apache/solr/cli/ExportTool.java:
##########
@@ -841,7 +832,14 @@ static long getDocCount(String coreName, SolrClient
client, String query)
@Override
public int callTool() throws Exception {
- String url = resolveSolrUrl(credentialsOptions.credentials) + "/solr/" +
nameOpt;
+ if (!connectionOptions.hasExplicitConnection()) {
+ throw new IllegalArgumentException(
+ "Must specify a connection target via -s/--solr-connection,
--solr-url, or --zk-host.");
+ }
Review Comment:
Same issue as `PostLogsTool`: this is a CLI usage error, but
`IllegalArgumentException` yields a non-usage failure path. Prefer throwing
`picocli.CommandLine.ParameterException` (with an injected `@CommandLine.Spec`)
so picocli renders usage consistently and returns `ExitCode.USAGE`.
##########
solr/core/src/java/org/apache/solr/cli/PostLogsTool.java:
##########
@@ -687,19 +678,16 @@ public static String[] getRequestPurposeNames(Integer
reqPurpose) {
@Override
public int callTool() throws Exception {
- String url = resolveSolrUrl(credentialsOptions.credentials) + "/solr/" +
nameOpt;
+ if (!connectionOptions.hasExplicitConnection()) {
+ throw new IllegalArgumentException(
+ "Must specify a connection target via -s/--solr-connection,
--solr-url, or --zk-host.");
+ }
Review Comment:
This is a usage/CLI input error, but throwing `IllegalArgumentException`
will be handled by `ToolBase` as a generic failure (exit code 1, no usage). To
align with the PR goal of having runtime validation reported like other invalid
inputs (exit code 2 + usage), throw `picocli.CommandLine.ParameterException`
instead (you’ll need access to the command’s `CommandSpec`/`CommandLine`, e.g.,
via `@CommandLine.Spec`).
##########
solr/core/src/java/org/apache/solr/cli/SolrCLI.java:
##########
@@ -188,6 +186,19 @@ public picocli.CommandLine.Help.IOptionRenderer
createDefaultOptionRenderer() {
}
/** Propagates common settings to all subcommands. */
+ /**
+ * The fully configured root command, as {@code bin/solr} runs it; tests
that drive a tool through
+ * the real root should use this too. A null factory means picocli's default
factory.
+ */
+ static picocli.CommandLine rootCommandLine(picocli.CommandLine.IFactory
factory) {
Review Comment:
There are two consecutive Javadoc blocks here; the first one is now detached
from `propagateCommandSettings(...)` and may trigger doclint/javadoc warnings
(“documentation comment not attached to any declaration”). Consider merging the
comments or moving the ‘Propagates common settings…’ Javadoc down to
immediately precede `propagateCommandSettings(...)`.
--
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]
