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

CLI Option reports arguments incorrectly on toString()

    Details

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

      Description

      The Option class contains some logic around the "hasArg" boolean that seems to be cruft and no longer works correctly. There is no need for this boolean anymore and its only usage could be replaced with hasArg() / hasArgs() calls.

      1. CLI-141-clean.patch
        2 kB
        Bjorn Townsend
      2. option.patch
        1 kB
        Henning Schmiedehausen

        Activity

        Hide
        henning Henning Schmiedehausen added a comment -

        Also contains serializable patch, apply with care.

        Show
        henning Henning Schmiedehausen added a comment - Also contains serializable patch, apply with care.
        Hide
        brianegge Brian Egge added a comment -

        I agree with the hasArg change. My preference for serializable classes is to use the number which would be generated by serialver. This way, if someone is already serializing this class, it won't break their code if they don't upgrade both the client and server at the same time.

        Other people prefer to start a 1L and increment each time the public interface changes. I don't know if Apache has a standard one way or the other.

        Show
        brianegge Brian Egge added a comment - I agree with the hasArg change. My preference for serializable classes is to use the number which would be generated by serialver. This way, if someone is already serializing this class, it won't break their code if they don't upgrade both the client and server at the same time. Other people prefer to start a 1L and increment each time the public interface changes. I don't know if Apache has a standard one way or the other.
        Hide
        henning Henning Schmiedehausen added a comment -

        You can not serialize the class currently. It is not serializable.

        It has no version, because it needs no version. I don't know anything about a serialization standard @ Apache, but I consider 1L and incrementing to be more readable.

        I don't really care about the version, as long as the classes become serializable. I have some time this weekend, so I could apply the patches, unless someone objects strongly.

        Show
        henning Henning Schmiedehausen added a comment - You can not serialize the class currently. It is not serializable. It has no version, because it needs no version. I don't know anything about a serialization standard @ Apache, but I consider 1L and incrementing to be more readable. I don't really care about the version, as long as the classes become serializable. I have some time this weekend, so I could apply the patches, unless someone objects strongly.
        Hide
        bayard Henri Yandell added a comment -

        Apply away

        Show
        bayard Henri Yandell added a comment - Apply away
        Hide
        henning Henning Schmiedehausen added a comment -

        Will do.

        Show
        henning Henning Schmiedehausen added a comment - Will do.
        Hide
        bayard Henri Yandell added a comment -

        Patch is wonky btw - it fails to apply against the branch, and when I look at it by hand, you've missed one of the hasArg usages.

        Show
        bayard Henri Yandell added a comment - Patch is wonky btw - it fails to apply against the branch, and when I look at it by hand, you've missed one of the hasArg usages.
        Hide
        bjorn Bjorn Townsend added a comment -

        Cleaned up the patch a little bit. It applies to the 1.x branch for me without any problems.

        The hasArg instance that Henri thought Henning missed is not an issue – it's just a local variable. +1 to adopting this.

        Show
        bjorn Bjorn Townsend added a comment - Cleaned up the patch a little bit. It applies to the 1.x branch for me without any problems. The hasArg instance that Henri thought Henning missed is not an issue – it's just a local variable. +1 to adopting this.
        Hide
        bayard Henri Yandell added a comment -

        Patch added: r596140.

        Show
        bayard Henri Yandell added a comment - Patch added: r596140.

          People

          • Assignee:
            Unassigned
            Reporter:
            henning Henning Schmiedehausen
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development