malliaridis commented on code in PR #2820:
URL: https://github.com/apache/solr/pull/2820#discussion_r1823372644
##########
solr/core/src/java/org/apache/solr/cli/ApiTool.java:
##########
@@ -57,37 +56,19 @@ public List<Option> getOptions() {
.longOpt("solr-url")
.argName("URL")
.hasArg()
- .required(false) // swap back to required when we eliminate
deprecated option
- .desc("Send a GET request to a Solr API endpoint.")
- .build(),
- Option.builder("get")
- .longOpt("get")
- .deprecated(
- DeprecatedAttributes.builder()
- .setForRemoval(true)
- .setSince("9.7")
- .setDescription("Use --solr-url instead")
- .get())
- .argName("URL")
- .hasArg()
- .required(false)
+ .required(true)
.desc("Send a GET request to a Solr API endpoint.")
.build(),
SolrCLI.OPTION_CREDENTIALS);
}
@Override
public void runImpl(CommandLine cli) throws Exception {
- String response = null;
- String getUrl =
- cli.hasOption("solr-url") ? cli.getOptionValue("solr-url") :
cli.getOptionValue("get");
- if (getUrl != null) {
- response = callGet(getUrl,
cli.getOptionValue(SolrCLI.OPTION_CREDENTIALS.getLongOpt()));
- }
- if (response != null) {
- // pretty-print the response to stdout
- echo(response);
- }
+ String getUrl = cli.getOptionValue("solr-url");
+ String response = callGet(getUrl,
cli.getOptionValue(SolrCLI.OPTION_CREDENTIALS.getLongOpt()));
+
+ // pretty-print the response to stdout
+ echo(response);
Review Comment:
What if `response` is `null`, should we print it anyway?
##########
solr/packaging/test/test_snapshots.bats:
##########
@@ -59,15 +59,15 @@ teardown() {
@test "snapshot list" {
solr snapshot-create -c films --snapshot-name snapshot3 --solr-url
http://localhost:${SOLR_PORT}
- run solr snapshot-list -c films -url http://localhost:${SOLR_PORT}/solr
+ run solr snapshot-list -c films -s http://localhost:${SOLR_PORT}/solr
Review Comment:
This may be a bit out of scope, but should `/solr` here be supported /
allowed? Because in my opinion the value passed to `-s` should be the same in
all commands, and right now we mix them. Additionally, `/solr` smells like v1
API, which is not future-proof probably.
##########
solr/core/src/java/org/apache/solr/cli/ApiTool.java:
##########
@@ -57,37 +56,19 @@ public List<Option> getOptions() {
.longOpt("solr-url")
.argName("URL")
.hasArg()
- .required(false) // swap back to required when we eliminate
deprecated option
- .desc("Send a GET request to a Solr API endpoint.")
- .build(),
- Option.builder("get")
- .longOpt("get")
- .deprecated(
- DeprecatedAttributes.builder()
- .setForRemoval(true)
- .setSince("9.7")
- .setDescription("Use --solr-url instead")
- .get())
- .argName("URL")
- .hasArg()
- .required(false)
+ .required(true)
.desc("Send a GET request to a Solr API endpoint.")
.build(),
SolrCLI.OPTION_CREDENTIALS);
}
@Override
public void runImpl(CommandLine cli) throws Exception {
- String response = null;
- String getUrl =
- cli.hasOption("solr-url") ? cli.getOptionValue("solr-url") :
cli.getOptionValue("get");
- if (getUrl != null) {
- response = callGet(getUrl,
cli.getOptionValue(SolrCLI.OPTION_CREDENTIALS.getLongOpt()));
- }
- if (response != null) {
- // pretty-print the response to stdout
- echo(response);
- }
+ String getUrl = cli.getOptionValue("solr-url");
Review Comment:
getUrl may be `null` here and throw `NullPointerException` further down.
##########
solr/core/src/java/org/apache/solr/cli/SolrCLI.java:
##########
@@ -215,18 +150,16 @@ public static void exit(int exitStatus) {
public static void main(String[] args) throws Exception {
final boolean hasNoCommand =
args == null || args.length == 0 || args[0] == null ||
args[0].trim().length() == 0;
- final boolean isHelpCommand =
- !hasNoCommand && Arrays.asList("-h", "--help", "/?").contains(args[0]);
+ final boolean isHelpCommand = !hasNoCommand && Arrays.asList("-h",
"--help").contains(args[0]);
if (hasNoCommand || isHelpCommand) {
printHelp();
exit(1);
}
- if (Arrays.asList("-version", "version").contains(args[0])) {
- // select the version tool to be run
- CLIO.out("Deprecated operation as of 9.8. Please use bin/solr
--version.");
- args = new String[] {"version"};
+ if (args[0].equalsIgnoreCase("version")) {
+ CLIO.out("version is not a valid command! Did you mean --version?\n");
+ exit(1);
Review Comment:
I' prefer to remove this case and allow the CLI to handle it like any other
case, since we do not have any "suggestions" behavior in general for any other
command.
--
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: issues-unsubscr...@solr.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org
For additional commands, e-mail: issues-h...@solr.apache.org
