You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@commons.apache.org by se...@apache.org on 2013/01/30 18:05:51 UTC
svn commit: r1440524 -
/commons/proper/cli/trunk/src/main/java/org/apache/commons/cli/Options.java
Author: sebb
Date: Wed Jan 30 17:05:51 2013
New Revision: 1440524
URL: http://svn.apache.org/viewvc?rev=1440524&view=rev
Log:
Document unexpected list contents
Modified:
commons/proper/cli/trunk/src/main/java/org/apache/commons/cli/Options.java
Modified: commons/proper/cli/trunk/src/main/java/org/apache/commons/cli/Options.java
URL: http://svn.apache.org/viewvc/commons/proper/cli/trunk/src/main/java/org/apache/commons/cli/Options.java?rev=1440524&r1=1440523&r2=1440524&view=diff
==============================================================================
--- commons/proper/cli/trunk/src/main/java/org/apache/commons/cli/Options.java (original)
+++ commons/proper/cli/trunk/src/main/java/org/apache/commons/cli/Options.java Wed Jan 30 17:05:51 2013
@@ -55,6 +55,8 @@ public class Options implements Serializ
private Map<String, Option> longOpts = new LinkedHashMap<String, Option>();
/** 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>();
/** a map of the option groups */
Re: svn commit: r1440524 - /commons/proper/cli/trunk/src/main/java/org/apache/commons/cli/Options.java
Posted by 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
Re: svn commit: r1440524 - /commons/proper/cli/trunk/src/main/java/org/apache/commons/cli/Options.java
Posted by Jörg Schaible <jo...@gmx.de>.
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):
================== %< =======================
Index: src/main/java/org/apache/commons/cli/DefaultParser.java
===================================================================
--- src/main/java/org/apache/commons/cli/DefaultParser.java (revision
1440679)
+++ src/main/java/org/apache/commons/cli/DefaultParser.java (working
copy)
@@ -54,7 +54,7 @@
protected boolean skipParsing;
/** The required options and groups expected to be found when parsing
the command line. */
- protected List expectedOpts;
+ protected List<OptionEntry> expectedOpts;
public CommandLine parse(Options options, String[] arguments) throws
ParseException
{
@@ -104,7 +104,7 @@
this.stopAtNonOption = stopAtNonOption;
skipParsing = false;
currentOption = null;
- expectedOpts = new ArrayList(options.getRequiredOptions());
+ expectedOpts = new
ArrayList<OptionEntry>(options.getRequiredOptions());
// clear the data from the groups
for (OptionGroup group : options.getOptionGroups())
@@ -624,7 +624,7 @@
{
if (option.isRequired())
{
- expectedOpts.remove(option.getKey());
+ expectedOpts.remove(option);
}
// if the option is in an OptionGroup make that option the selected
option of the group
Index: src/main/java/org/apache/commons/cli/MissingOptionException.java
===================================================================
--- src/main/java/org/apache/commons/cli/MissingOptionException.java
(revision 1440679)
+++ src/main/java/org/apache/commons/cli/MissingOptionException.java
(working copy)
@@ -17,6 +17,7 @@
package org.apache.commons.cli;
+import java.util.ArrayList;
import java.util.List;
import java.util.Iterator;
@@ -32,7 +33,7 @@
private static final long serialVersionUID = 8161889051578563249L;
/** The list of missing options and groups */
- private List missingOptions;
+ private List<Object> missingOptions;
/**
* Construct a new <code>MissingSelectedException</code>
@@ -52,10 +53,13 @@
* @param missingOptions the list of missing options and groups
* @since 1.2
*/
- public MissingOptionException(List missingOptions)
+ public MissingOptionException(List<OptionEntry> missingOptions)
{
this(createMessage(missingOptions));
- this.missingOptions = missingOptions;
+ this.missingOptions = new ArrayList<Object>(missingOptions.size());
+ for (OptionEntry entry : missingOptions) {
+ this.missingOptions.add((entry instanceof Option ?
((Option)entry).getKey() : entry));
+ }
}
/**
@@ -65,7 +69,7 @@
* options, and OptionGroup instances for required option
groups.
* @since 1.2
*/
- public List getMissingOptions()
+ public List<?> getMissingOptions()
{
return missingOptions;
}
@@ -76,16 +80,17 @@
* @param missingOptions the list of missing options and groups
* @since 1.2
*/
- private static String createMessage(List<?> missingOptions)
+ private static String createMessage(List<OptionEntry> missingOptions)
{
StringBuilder buf = new StringBuilder("Missing required option");
buf.append(missingOptions.size() == 1 ? "" : "s");
buf.append(": ");
- Iterator<?> it = missingOptions.iterator();
+ Iterator<OptionEntry> it = missingOptions.iterator();
while (it.hasNext())
{
- buf.append(it.next());
+ OptionEntry next = it.next();
+ buf.append(next instanceof Option ? ((Option)next).getKey() :
next);
if (it.hasNext())
{
buf.append(", ");
Index: src/main/java/org/apache/commons/cli/Option.java
===================================================================
--- src/main/java/org/apache/commons/cli/Option.java (revision 1440679)
+++ src/main/java/org/apache/commons/cli/Option.java (working copy)
@@ -36,7 +36,7 @@
* @author <a href="mailto:jstrachan@apache.org">James Strachan</a>
* @version $Revision$, $Date$
*/
-public class Option implements Cloneable, Serializable
+public class Option implements Cloneable, Serializable, OptionEntry
{
/** constant that specifies the number of argument values has not been
specified */
public static final int UNINITIALIZED = -1;
Index: src/main/java/org/apache/commons/cli/OptionEntry.java
===================================================================
--- src/main/java/org/apache/commons/cli/OptionEntry.java (revision 0)
+++ src/main/java/org/apache/commons/cli/OptionEntry.java (working
copy)
@@ -0,0 +1,26 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.commons.cli;
+
+/**
+ * Internal marker interface for Option and OptionGroup instances.
+ *
+ * @version $Revision$, $Date$
+ */
+interface OptionEntry {
+ // marker only
+}
Property changes on: src/main/java/org/apache/commons/cli/OptionEntry.java
___________________________________________________________________
Added: svn:keywords
## -0,0 +1 ##
+Author Date Id HeadURL Revision
\ No newline at end of property
Added: svn:eol-style
## -0,0 +1 ##
+native
\ No newline at end of property
Index: src/main/java/org/apache/commons/cli/OptionGroup.java
===================================================================
--- src/main/java/org/apache/commons/cli/OptionGroup.java (revision
1440679)
+++ src/main/java/org/apache/commons/cli/OptionGroup.java (working
copy)
@@ -29,7 +29,7 @@
* @author John Keyes ( john at integralsource.com )
* @version $Revision$, $Date$
*/
-public class OptionGroup implements Serializable
+public class OptionGroup implements Serializable, OptionEntry
{
/** The serial version UID. */
private static final long serialVersionUID = 1L;
Index: src/main/java/org/apache/commons/cli/Options.java
===================================================================
--- src/main/java/org/apache/commons/cli/Options.java (revision 1440679)
+++ src/main/java/org/apache/commons/cli/Options.java (working copy)
@@ -57,7 +57,7 @@
/** 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>();
+ private List<OptionEntry> requiredOpts = new ArrayList<OptionEntry>();
/** a map of the option groups */
private Map<String, OptionGroup> optionGroups = new HashMap<String,
OptionGroup>();
@@ -151,11 +151,11 @@
// if the option is required add it to the required list
if (opt.isRequired())
{
- if (requiredOpts.contains(key))
+ if (requiredOpts.contains(opt))
{
- requiredOpts.remove(requiredOpts.indexOf(key));
+ requiredOpts.remove(requiredOpts.indexOf(opt));
}
- requiredOpts.add(key);
+ requiredOpts.add(opt);
}
shortOpts.put(key, opt);
@@ -188,7 +188,7 @@
*
* @return List of required options
*/
- public List getRequiredOptions()
+ public List<OptionEntry> getRequiredOptions()
{
return requiredOpts;
}
Index: src/main/java/org/apache/commons/cli/Parser.java
===================================================================
--- src/main/java/org/apache/commons/cli/Parser.java (revision 1440679)
+++ src/main/java/org/apache/commons/cli/Parser.java (working copy)
@@ -41,12 +41,12 @@
private Options options;
/** list of required options strings */
- private List requiredOptions;
+ private List<OptionEntry> requiredOptions;
protected void setOptions(Options options)
{
this.options = options;
- this.requiredOptions = new ArrayList(options.getRequiredOptions());
+ this.requiredOptions = new
ArrayList<OptionEntry>(options.getRequiredOptions());
}
protected Options getOptions()
@@ -54,7 +54,7 @@
return options;
}
- protected List getRequiredOptions()
+ protected List<OptionEntry> getRequiredOptions()
{
return requiredOptions;
}
@@ -407,7 +407,7 @@
// the requiredOptions list
if (opt.isRequired())
{
- getRequiredOptions().remove(opt.getKey());
+ getRequiredOptions().remove(opt);
}
// if the option is in an OptionGroup make that option the selected
================== %< =======================
- Jörg
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org
Re: svn commit: r1440524 - /commons/proper/cli/trunk/src/main/java/org/apache/commons/cli/Options.java
Posted by sebb <se...@gmail.com>.
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.
> - Jörg
>
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
> For additional commands, e-mail: dev-help@commons.apache.org
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org
Re: svn commit: r1440524 - /commons/proper/cli/trunk/src/main/java/org/apache/commons/cli/Options.java
Posted by Jörg Schaible <jo...@gmx.de>.
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.
- Jörg
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org
Re: svn commit: r1440524 - /commons/proper/cli/trunk/src/main/java/org/apache/commons/cli/Options.java
Posted by sebb <se...@gmail.com>.
On 30 January 2013 17:12, Thomas Neidhart <th...@gmail.com> wrote:
> On Wed, Jan 30, 2013 at 6:05 PM, <se...@apache.org> wrote:
>
>> Author: sebb
>> Date: Wed Jan 30 17:05:51 2013
>> New Revision: 1440524
>>
>> URL: http://svn.apache.org/viewvc?rev=1440524&view=rev
>> Log:
>> Document unexpected list contents
>>
>> Modified:
>>
>> commons/proper/cli/trunk/src/main/java/org/apache/commons/cli/Options.java
>>
>> Modified:
>> commons/proper/cli/trunk/src/main/java/org/apache/commons/cli/Options.java
>> URL:
>> http://svn.apache.org/viewvc/commons/proper/cli/trunk/src/main/java/org/apache/commons/cli/Options.java?rev=1440524&r1=1440523&r2=1440524&view=diff
>>
>> ==============================================================================
>> ---
>> commons/proper/cli/trunk/src/main/java/org/apache/commons/cli/Options.java
>> (original)
>> +++
>> commons/proper/cli/trunk/src/main/java/org/apache/commons/cli/Options.java
>> Wed Jan 30 17:05:51 2013
>> @@ -55,6 +55,8 @@ public class Options implements Serializ
>> private Map<String, Option> longOpts = new LinkedHashMap<String,
>> Option>();
>>
>> /** 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 ...
> Thomas
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org
Re: svn commit: r1440524 - /commons/proper/cli/trunk/src/main/java/org/apache/commons/cli/Options.java
Posted by Thomas Neidhart <th...@gmail.com>.
On Wed, Jan 30, 2013 at 6:05 PM, <se...@apache.org> wrote:
> Author: sebb
> Date: Wed Jan 30 17:05:51 2013
> New Revision: 1440524
>
> URL: http://svn.apache.org/viewvc?rev=1440524&view=rev
> Log:
> Document unexpected list contents
>
> Modified:
>
> commons/proper/cli/trunk/src/main/java/org/apache/commons/cli/Options.java
>
> Modified:
> commons/proper/cli/trunk/src/main/java/org/apache/commons/cli/Options.java
> URL:
> http://svn.apache.org/viewvc/commons/proper/cli/trunk/src/main/java/org/apache/commons/cli/Options.java?rev=1440524&r1=1440523&r2=1440524&view=diff
>
> ==============================================================================
> ---
> commons/proper/cli/trunk/src/main/java/org/apache/commons/cli/Options.java
> (original)
> +++
> commons/proper/cli/trunk/src/main/java/org/apache/commons/cli/Options.java
> Wed Jan 30 17:05:51 2013
> @@ -55,6 +55,8 @@ public class Options implements Serializ
> private Map<String, Option> longOpts = new LinkedHashMap<String,
> Option>();
>
> /** 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.
Thomas