malliaridis commented on code in PR #2725:
URL: https://github.com/apache/solr/pull/2725#discussion_r1831113411
##########
solr/CHANGES.txt:
##########
@@ -26,6 +26,8 @@ Improvements
* SOLR-16116: Apache Curator is now used to manage all Solr Zookeeper
interactions. This should provide more stability in the Solr-Zookeeper
interactions.
The solrj-zookeeper module, now has a dependency on curator. (Houston
Putman, Kevin Risden, Mike Drob, David Smiley)
+* SOLR-17544: Solr CLI will now warn you when you use multiple options and
only one is a valid choice. Combining -s and -z is most common example. (Eric
Pugh, Christos Malliaridis)
Review Comment:
The OptionGroup interrupts as far as I know, "warn" is mildly expressed.
```suggestion
* SOLR-17544: Solr CLI will stop when you combine mutually exclusive
options. Combining -s and -z is most common example. (Eric Pugh, Christos
Malliaridis)
```
##########
solr/core/src/java/org/apache/solr/cli/ToolBase.java:
##########
@@ -48,9 +52,30 @@ protected void echo(final String msg) {
stdout.println(msg);
}
+ @Override
+ public Options getAllOptions() {
+ return new Options()
+ .addOption(CommonCLIOptions.HELP_OPTION)
+ .addOption(CommonCLIOptions.VERBOSE_OPTION);
+ }
+
+ @Override
+ public Options getOptions() {
+ Options options = new Options();
+
+ Collection<Option> toolOpts = getAllOptions().getOptions();
+ for (Option toolOpt : toolOpts) {
+ if (!toolOpt.isDeprecated()) {
+ options.addOption(toolOpt);
+ }
+ }
+ return options;
+ }
+
@Override
public int runTool(CommandLine cli) throws Exception {
- verbose = cli.hasOption(SolrCLI.OPTION_VERBOSE.getLongOpt());
+ verbose = cli.hasOption(CommonCLIOptions.VERBOSE_OPTION);
Review Comment:
I would probably prefer the option group to be placed in `CommonCLIOptions`
as another "option", mainly because of context, class responsibilities and for
later referencing in other classes. I believe that there won't be any overrides
either in implementations of `ToolBase`.
```java
// If CommonCLIOptions only imported
super.getOptions()
.addOptionGroup(CommonCLIOptions.CONNECTION_OPTIONS)
...
if (cli.hasOption(CommonCLIOptions.CONNECTION_OPTIONS) { ... }
```
`getConnectionOptions` being placed in `ToolBase` reminds me on
`getOptions()` and I would expect `Options` to be returned, rather than an
`OptionGroup`.
--
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]
