Uploaded image for project: 'Commons CLI'
  1. Commons CLI
  2. CLI-224

OptionBuilder only has static methods, yet many return an OptionBuilder instance

    Details

    • Type: Bug
    • Status: Closed
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: 1.2
    • Fix Version/s: 1.3
    • Component/s: CLI-1.x
    • Labels:
      None

      Description

      CLI-83 was closed as fixed in 2.0, but it seems that there is no current plan to release CLI 2.0. Could this fix be rolled into the 1.x line? I'm attaching a new version of option builder that addresses the issue (named DefaultOptionBuilder to maintain backwards compatability) and a unit test for it.

      1. commons-cli-CLI-224.patch
        11 kB
        Duncan Jones
      2. DefaultOptionBuilderTest.java
        4 kB
        Brian Blount
      3. DefaultOptionBuilder.java
        11 kB
        Brian Blount

        Issue Links

          Activity

          Hide
          ebourg Emmanuel Bourg added a comment -

          Thank you for the code Brian, a revamped builder could be included in the 1.x line, but I'm still hesitating on the proper way do to it. The existing OptionBuilder can't be changed due to binary compatibility issues, so another class has to provide the builder mechanism. I haven't found an elegant solution yet: "DefaultOptionBuilder" looks a bit heavy, "Options" is already used, "OptBuilder" is shorter but probably less explicit.

          The solution might be to add chained withXXX() setters to the Option class. Building an option would then look like this:

          new Option("f").withLongOpt("file").withDescription("Input file").withArg();

          Any input on this matter is welcome.

          Show
          ebourg Emmanuel Bourg added a comment - Thank you for the code Brian, a revamped builder could be included in the 1.x line, but I'm still hesitating on the proper way do to it. The existing OptionBuilder can't be changed due to binary compatibility issues, so another class has to provide the builder mechanism. I haven't found an elegant solution yet: "DefaultOptionBuilder" looks a bit heavy, "Options" is already used, "OptBuilder" is shorter but probably less explicit. The solution might be to add chained withXXX() setters to the Option class. Building an option would then look like this: new Option( "f" ).withLongOpt( "file" ).withDescription( "Input file" ).withArg(); Any input on this matter is welcome.
          Hide
          dmjones500 Duncan Jones added a comment - - edited

          How about embedding a Builder class within Option and deprecating the public Option constructors and the OptionBuilder class. Hence we can:

          new Option.Builder("opt", "description").argName("Foo").build();
          

          without impacting existing code. (This embedded builder style is suggested by Joshua Bloch in Effective Java.)

          Show
          dmjones500 Duncan Jones added a comment - - edited How about embedding a Builder class within Option and deprecating the public Option constructors and the OptionBuilder class. Hence we can: new Option.Builder( "opt" , "description" ).argName( "Foo" ).build(); without impacting existing code. (This embedded builder style is suggested by Joshua Bloch in Effective Java.)
          Hide
          dmjones500 Duncan Jones added a comment - - edited

          Attached is an alternative patch (commons-cli-CLI-224.patch), based on my suggestion of a nested builder class. I must thank Brian for the design of the unit test, which I have used in my patch.

          I welcome comments.

          Show
          dmjones500 Duncan Jones added a comment - - edited Attached is an alternative patch (commons-cli- CLI-224 .patch), based on my suggestion of a nested builder class. I must thank Brian for the design of the unit test, which I have used in my patch. I welcome comments.
          Hide
          dmjones500 Duncan Jones added a comment -

          Corrected patch (missed Apache licence statement in new unit test).

          Show
          dmjones500 Duncan Jones added a comment - Corrected patch (missed Apache licence statement in new unit test).
          Hide
          brian.blount@zilliant.com Brian Blount added a comment -

          Duncan, this approach looks great to me. Will there be a new release of 1.x with this change?

          Show
          brian.blount@zilliant.com Brian Blount added a comment - Duncan, this approach looks great to me. Will there be a new release of 1.x with this change?
          Hide
          dmjones500 Duncan Jones added a comment -

          I've emailed the developer list to seek feedback on this issue. Hopefully it will result in somebody reviewing the patches and committing something to trunk. As for the next release, I'm not sure the plans for this, sorry.

          Show
          dmjones500 Duncan Jones added a comment - I've emailed the developer list to seek feedback on this issue. Hopefully it will result in somebody reviewing the patches and committing something to trunk. As for the next release, I'm not sure the plans for this, sorry.
          Hide
          tn Thomas Neidhart added a comment -

          I like the second patch too, it is much cleaner than the OptionBuilder we have now in place.
          I am going to integrate this patch and deprecate the OptionBuilder as a consequence.

          Show
          tn Thomas Neidhart added a comment - I like the second patch too, it is much cleaner than the OptionBuilder we have now in place. I am going to integrate this patch and deprecate the OptionBuilder as a consequence.
          Hide
          tn Thomas Neidhart added a comment -

          Applied patch in r1444720 with minor modifications (javadoc, formatting).

          Thanks for the patch!

          Show
          tn Thomas Neidhart added a comment - Applied patch in r1444720 with minor modifications (javadoc, formatting). Thanks for the patch!
          Hide
          tn Thomas Neidhart added a comment - - edited

          Added also a default constructor for Option.Builder(), as the current OptionBuilder also allows the following:

            OptionBuilder.withLongOpt("v").create()
          

          with the Builder it will be like this:

            new Option.Builder().longOpt("v").build();
          

          What about a static factory method, as always typing new may be tedious?

            Option.builder().longOpt("v")......build();
          
          Show
          tn Thomas Neidhart added a comment - - edited Added also a default constructor for Option.Builder(), as the current OptionBuilder also allows the following: OptionBuilder.withLongOpt("v").create() with the Builder it will be like this: new Option.Builder().longOpt("v").build(); What about a static factory method, as always typing new may be tedious? Option.builder().longOpt("v")......build();
          Hide
          dmjones500 Duncan Jones added a comment -

          What about a static factory method, as always typing new may be tedious?

            Option.builder().longOpt("v")......build();
          

          Yes, that's a good idea. Do you propose to make the Builder constructors private or are you leaving those as an alternative?

          Show
          dmjones500 Duncan Jones added a comment - What about a static factory method, as always typing new may be tedious? Option.builder().longOpt( "v" )......build(); Yes, that's a good idea. Do you propose to make the Builder constructors private or are you leaving those as an alternative?
          Hide
          tn Thomas Neidhart added a comment -

          Yes, I think the Builder ctors should be private.

          Show
          tn Thomas Neidhart added a comment - Yes, I think the Builder ctors should be private.
          Hide
          tn Thomas Neidhart added a comment -

          Applied changes as discussed on ml in r1447005.

          Show
          tn Thomas Neidhart added a comment - Applied changes as discussed on ml in r1447005.

            People

            • Assignee:
              Unassigned
              Reporter:
              brian.blount@zilliant.com Brian Blount
            • Votes:
              0 Vote for this issue
              Watchers:
              3 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development