Re: svn commit: r1440524 - /commons/proper/cli/trunk/src/main/java/org/apache/commons/cli/Options.java

[Thomas Neidhart <th...@gmail.com>]

On 01/31/2013 12:01 AM, Jörg Schaible wrote:
> sebb wrote:
> 
>> On 30 January 2013 18:18, Jörg Schaible <jo...@gmx.de> wrote:
>>> Hi,
>>>
>>> sebb wrote:
>>>
>>> [snip]
>>>
>>>>>>      /** a map of the required options */
>>>>>> +    // N.B. This can contain either a String (addOption) or an
>>>>>> OptionGroup (addOptionGroup)
>>>>>> +    // TODO this seems wrong
>>>>>>      private List<Object> requiredOpts = new ArrayList<Object>();
>>>>>>
>>>>>
>>>>> Indeed, I also spotted this and failed to resolve it, as the logic in
>>>>> the parsers is somehow taken advantage of it in a way I do not yet
>>>>> fully understand.
>>>>
>>>> Me neither.
>>>>
>>>> Maybe the code would still work if the entries were always OptionGroups.
>>>> This could perhaps be done by converting the Option into a
>>>> single-entry OptionGroup and storing that, rather than storing the
>>>> Option key String.
>>>> In theory that might work ...
>>>
>>> Or create a package local marker interface OptionEntry and let both
>>> classes implement it.
>>
>> Not possible, because String is final.
>>
>> One might try to store the Option instead of its key (String).
>>
>> However, the key is used to avoid duplicate entries - addOption()
>> removes any entry with a matching key.
>> This would be difficult to do with Option entries.
>> Option instances with equal keys are not necessarily equal, though the
>> reverse is true.
>>
>> Also Option instances are not immutable (in fact the key is not even
>> immutable - longOpt can be changed after instantiation).
>> All a bit messsy.
> 
> IMHO, the *intent* was clear. With the following diff we could introduce an 
> OptionEntry, although none of the mentioned problems of an Option instance 
> is really solved (all tests run, but it's not completely binary compatible):

[snip]

I think for 1.3 we have to keep it as it is. The required options can be
retrieved with a public/protected method in the Options and Parser
class. Thus if we decide to change the content of the list, it will
break compatibility.

Thomas

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org