malliaridis commented on code in PR #2721: URL: https://github.com/apache/solr/pull/2721#discussion_r1772845049
########## solr/core/src/java/org/apache/solr/cli/SolrCLI.java: ########## @@ -148,9 +154,21 @@ public class SolrCLI implements CLIO { public static final Option OPTION_VERBOSE = Option.builder("v") .longOpt("verbose") + .deprecated( + DeprecatedAttributes.builder() + .setForRemoval(true) + .setSince("9.8") + .setDescription("Use --debug instead") + .get()) .required(false) .desc("Enable verbose command output.") .build(); + public static final Option OPTION_DEBUG = + Option.builder("d") + .longOpt("debug") + .required(false) + .desc("Enable additional command output.") + .build(); Review Comment: I wasn't sure if we would go for the `--debug` option. Shall we also deprecate the `-d` for all the other arguments as well in this PR? We use `-d` in quite a few places and providing the `-d` now for `debug` to all tools may cause unexpected behavior. ########## solr/core/src/java/org/apache/solr/cli/SolrCLI.java: ########## @@ -148,9 +154,21 @@ public class SolrCLI implements CLIO { public static final Option OPTION_VERBOSE = Review Comment: Do we mark deprecated options with `@Deprecated`? If so, this would be beneficial for `OPTION_VERBOSE`. ########## solr/core/src/java/org/apache/solr/cli/SolrCLI.java: ########## @@ -137,6 +137,12 @@ public class SolrCLI implements CLIO { public static final Option OPTION_SOLRURL = Option.builder("url") .longOpt("solr-url") + .deprecated( + DeprecatedAttributes.builder() + .setForRemoval(true) + .setSince("9.7") + .setDescription("Use --solr-url instead") + .get()) Review Comment: Leaving this as a note: Splitting `url` from `solr-url` would have a way too large impact in the rest of the tools, so simply marking both as deprecated is a good tradeoff. ########## solr/core/src/java/org/apache/solr/cli/SolrCLI.java: ########## @@ -308,7 +326,8 @@ protected static void checkSslStoreSysProp(String solrInstallDir, String key) { } public static void raiseLogLevelUnlessVerbose(CommandLine cli) { - if (!cli.hasOption(OPTION_VERBOSE.getOpt())) { + if (!cli.hasOption((SolrCLI.OPTION_DEBUG.getOpt())) + || cli.hasOption(SolrCLI.OPTION_VERBOSE.getOpt())) { Review Comment: Have you considered putting both options (verbose and debug) into an `OptionGroup` (mutually exclusive)? This would allow single option (group) checks. -- 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