[CLI-329] Support "Deprecated" CLI Options

[garydgregory]

https://issues.apache.org/jira/browse/CLI-329

Adds support to allow deprecation of Option objects.

A simple example:

final Option opt1 = Option.builder().option("d1").deprecated().build();

A more detailed example:

final Option opt2 = Option.builder().option("d2").deprecated(DeprecatedAttributes.builder()
        .setForRemoval(true)      // Like the Deprecated annotation
        .setSince("1.0")          // Like the Deprecated annotation
        .setDescription("Do this instead.").get()).build(); // Like the Deprecated Javadoc tag

[codecov-commenter]

Codecov Report

Attention: Patch coverage is 98.55072% with 1 lines in your changes are missing coverage. Please review.

Project coverage is 92.33%. Comparing base (9f6b23b) to head (c1b00a8).
Report is 58 commits behind head on master.

Files	Patch %	Lines
.../main/java/org/apache/commons/cli/CommandLine.java	92.30%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##             master     #252      +/-   ##
============================================
+ Coverage     91.90%   92.33%   +0.43%     
- Complexity      575      612      +37     
============================================
  Files            22       23       +1     
  Lines          1247     1331      +84     
  Branches        210      218       +8     
============================================
+ Hits           1146     1229      +83     
+ Misses           63       60       -3     
- Partials         38       42       +4     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[epugh]

I will give this a stab... so the idea is to add two seperate options, the deprecated one as described above and the non deprecated one...?

[garydgregory]

Hi @epugh
Yes, just like an API, you have the existing option, you mark that one deprecated, then you create your new option.

[epugh]

So, one thing I didn't love is that now I get duplicated options when I run my -h command. Here is the output, and you can see the deprecated zkHost along with the the current -z and --zk-host option:

➜  dev git:(SOLR-16824) ✗ ./bin/solr create -h
usage: create
 -c,--name <NAME>                 Name of collection or core to create.
 -d,--confdir <NAME>              Configuration directory to copy when
                                  creating the new collection; default is
                                  _default.
 -h,--help                        Print this message.
 -n,--confname <NAME>             Configuration name; default is the
                                  collection name.
 -rf,--replication-factor <#>     Number of copies of each document across
                                  the collection (replicas per shard);
                                  default is 1.
 -s,--shards <#>                  Number of shards; default is 1.
 -u,--credentials <credentials>   Credentials in the format
                                  username:password. Example:
                                  --credentials solr:SolrRocks
 -url,--solr-url <HOST>           Base Solr URL, which can be used to
                                  determine the zk-host if that's not
                                  known; defaults to:
                                  http://localhost:8983.
 -v,--verbose                     Enable more verbose command output.
 -z,--zk-host <HOST>              Zookeeper connection string; unnecessary
                                  if ZK_HOST is defined in solr.in.sh;
                                  otherwise, defaults to localhost:9983.
 -zkHost,--zkHost <HOST>          Zookeeper connection string; unnecessary
                                  if ZK_HOST is defined in solr.in.sh;
                                  otherwise, defaults to localhost:9983.

Here is the commit where I tried this approach, maybe some other ways? apache/solr@ac699bc

I wonder if we should change HelpFormatter to filter deprecated options?
This is what we use in SolrCLI.java to print out the help:

HelpFormatter formatter = new HelpFormatter();
formatter.printHelp(toolName, options);

[garydgregory]

I also forgot to make the help formatter say an option is deprecated. Let me have a look...

[epugh]

One more wrinkle in having the seperate options is that we need to remember to check both...

We look up zkhost like this:

String zkHost = cli.getOptionValue("z");

and would need to check the cli.getOptionValue("zkHost") as well if they are using the deprecated approach.... Which maybe is okay, it's just more logic...

[garydgregory]

The lastest:

• HelpFormatter does not print Deprecated options by default
• HelpFormatter can show deprecated options when you build your formatter to do so like this:
  HelpFormatter.builder().setShowDeprecated(true).get();

Reading your latest comment #252 (comment)

[garydgregory]

One more wrinkle in having the seperate options is that we need to remember to check both...

I think that's fair, an application should decide what exact behavior it wants for deprecated options.

Let me know what you think.

[epugh]

One more wrinkle in having the seperate options is that we need to remember to check both...

I think that's fair, an application should decide what exact behavior it wants for deprecated options.

Let me know what you think.

That works!

[epugh]

Okay, so I did:

 String zkHost = cli.hasOption("z") ? cli.getOptionValue("z") : cli.getOptionValue("zkHost");

And I see deprecation message:

[ Deprecated for removal since 9.6 option: zkHost zkHost  [ARG] :: Zookeeper connection string; unnecessary if ZK_HOST is defined in solr.in.sh; otherwise, defaults to localhost:9983. :: class java.lang.String ]

[epugh]

I wonder if the deprecation message could be clearer and more concise. "[ Option 'zkHost' deprecated for removal since 9.6. Use --zk-host instead.]. where the Use --zk-host instead is the setDescription()?

[garydgregory]

I wonder if the deprecation message could be clearer and more concise. "[ Option 'zkHost' deprecated for removal since 9.6. Use --zk-host instead.]. where the Use --zk-host instead is the setDescription()?

@epugh

I updated the default deprecated string. You can customize the string by replacing the existing deprecated handler through DefaultParser.Builder.setDeprecatedHandler(Consumer<Option>).

The default handler is just o -> System.out.println(o.toDeprecatedString());

[garydgregory]

Hi @epugh
Are we all set?

[epugh]

@garydgregory I am going to close my PR in favour of this one... What can I do to help move this along?

[garydgregory]

@epugh
We're all set then, merged!