commons-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Jörg Schaible <joerg.schai...@gmx.de>
Subject Re: svn commit: r1440524 - /commons/proper/cli/trunk/src/main/java/org/apache/commons/cli/Options.java
Date Wed, 30 Jan 2013 23:01:03 GMT
sebb wrote:

> On 30 January 2013 18:18, Jörg Schaible <joerg.schaible@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


Mime
View raw message