fix @boegel's remarks ;-)

lib/vsc/utils/generaloption.py

@@ -227,7 +227,7 @@ def process(self, opt, value, values, parser):
         """Handle option-as-value issues before actually processing option."""
 
         if hasattr(parser, 'is_value_a_commandline_option'):
-            errmsg = parser.is_value_a_commandline_option(value)
+            errmsg = parser.is_value_a_commandline_option(opt, value)
             if errmsg is not None:
                 prefix = "%s=" % self._long_opts[0] if self._long_opts else self._short_opts[0]
                 self.log.raiseException("%s. Use '%s%s' if the value is correct." % (errmsg, prefix, value),
@@ -432,9 +432,10 @@ class ExtOptionParser(OptionParser):
     VALUES_CLASS = Values
     DESCRIPTION_DOCSTRING = False
 
-    ALLOW_OPTION_AS_VALUE = False # exact match for option as value
-    ALLOW_DASH_AS_VALUE = False # any value starting with a '-'
-    ALLOW_TYPO_AS_VALUE = True # value with similarity score from difflib.get_close_matches
+    ALLOW_OPTION_NAME_AS_VALUE = False  # exact match for option name (without the '-') as value
+    ALLOW_OPTION_AS_VALUE = False  # exact match for option as value
+    ALLOW_DASH_AS_VALUE = False  # any value starting with a '-'
+    ALLOW_TYPO_AS_VALUE = True  # value with similarity score from difflib.get_close_matches
 
     def __init__(self, *args, **kwargs):
         """
@@ -488,38 +489,49 @@ def __init__(self, *args, **kwargs):
         self.environment_arguments = None
         self.commandline_arguments = None
 
-    def is_value_a_commandline_option(self, value, index=None):
+    def is_value_a_commandline_option(self, opt, value, index=None):
         """
         Determine if value is/could be an option passed via the commandline.
         If it is, return the reason why (can be used as message); or return None if it isn't.
 
+        opt is the option flag to which the value is passed;
         index is the index of the value on the commandline (if None, it is determined from orig_rargs and rargs)
 
         The method tests for possible ambiguity on the commandline when the parser
         interprets the argument following an option as a value, whereas it is far more likely that
-        it is (intended as) an option.
+        it is (intended as) an option; --longopt=value is never considered ambiguous, regardless of the value.
         """
         # Values that are/could be options that are passed via
-        # --longopt=value or -sVALUE are not a problem.
+        # only --longopt=value is not a problem.
         # When processing the enviroment and/or configfile, we always set
         # --longopt=value, so no issues there either.
 
+        # following checks assume that value is a string (not a store_or_None)
+        if not isinstance(value, basestring):
+            return None
+
+        cmdline_index = None
         try:
             cmdline_index = self.commandline_arguments.index(value)
         except ValueError:
-            # There is no ambiguity if the value is not passed
-            # as standalone argument via commandline
-            return None
+            # no index found for value, so not a stand-alone value
+            if opt.startswith('--'):
+                # only --longopt=value is unambigouos
+                return None
 
         if index is None:
-            # index of last parsed arg in orig_rargs via remainder of rargs
+            # index of last parsed arg in commandline_arguments via remainder of rargs
             index = len(self.commandline_arguments) - len(self.rargs) - 1
 
-        if index != cmdline_index:
+        if cmdline_index is not None and index != cmdline_index:
             # This is not the value you are looking for
             return None
 
-        # First test the exact problem where the value is an option
+        if not self.ALLOW_OPTION_NAME_AS_VALUE:
+            value_as_opt = '-%s' % value
+            if value_as_opt in self._short_opt or value_as_opt in self._long_opt:
+                return "'-%s' is a valid option" % value
+
         if (not self.ALLOW_OPTION_AS_VALUE) and (value in self._long_opt or value in self._short_opt):
             return "Value '%s' is also a valid option" % value
 

test/generaloption.py

@@ -774,9 +774,18 @@ def raise_error(msg, *args):
                               error_env_option_method=raise_error)
 
     def _match_testoption1_sysexit(self, args, msg):
+        """Check whether parsing supplied arguments with TestOption1 results in SystemExit error being raised."""
+        system_exit = False
         self.mock_stderr(True)  # reset
-        self.assertErrorRegex(SystemExit, '.*', TestOption1, go_args=args)
-        stderr = self.get_stderr()
+        # purposely not using self.assertErrorRegex, since we're mocking stderr...
+        try:
+            TestOption1(go_args=args)
+        except SystemExit:
+            system_exit = True
+        finally:
+            stderr = self.get_stderr()
+            self.mock_stderr(False)
+        self.assertTrue(system_exit)
         self.assertTrue(msg in stderr)
 
     def test_nosuchoption(self):
@@ -788,64 +797,80 @@ def test_is_value_a_commandline_option(self):
         """Test ExtOptionParser is_value_a_commandline_option method"""
         topt = TestOption1(go_args=[])
         tp = topt.parser
-        fn = tp.is_value_a_commandline_option
 
+        self.assertEqual(tp.ALLOW_OPTION_NAME_AS_VALUE, False,
+                         msg="default do not allow option name as value")
         self.assertEqual(tp.ALLOW_OPTION_AS_VALUE, False,
-                         msg="default do not allow value as option")
+                         msg="default do not allow option as value")
         self.assertEqual(tp.ALLOW_DASH_AS_VALUE, False,
                          msg="default do not allow value starting with -")
         self.assertEqual(tp.ALLOW_TYPO_AS_VALUE, True,
                          msg="default do allow value similar to option")
 
         # fake commandline args
         # this is purely to test the function, actual usage is in test_option_as_value
-        tp.commandline_arguments = ['--base', '-something', 'base']
-        def t(fail, tpl):
+        tp.commandline_arguments = ['--base', '-something', 'base', '-base']
+        def check(fail, tpl):
+            """Check whether expected failures/success occur with given message template."""
             for idx, value in enumerate(tp.commandline_arguments):
                 msg = tpl % value
-                res = fn(value, index=idx)
+                res = tp.is_value_a_commandline_option('--opt', value, index=idx)
                 if idx in fail:
                     self.assertFalse(res is None, "failure should not return None for value %s" % value)
                     self.assertTrue(msg in res, msg='%s in %s' % (msg, res))
                 else:
                     self.assertTrue(res is None)
 
         # anything goes
+        tp.ALLOW_OPTION_NAME_AS_VALUE = True
         tp.ALLOW_OPTION_AS_VALUE = True
         tp.ALLOW_DASH_AS_VALUE = True
         tp.ALLOW_TYPO_AS_VALUE = True
 
-        t([-1], 'x%sx') # all ok
+        check([], 'x%sx') # all ok
 
         tp.ALLOW_OPTION_AS_VALUE = False
-        t([0], "Value '%s' is also a valid option")
+        check([0], "Value '%s' is also a valid option")
         tp.ALLOW_OPTION_AS_VALUE = True
 
         tp.ALLOW_DASH_AS_VALUE = False
         # options also start with a -
-        t([0, 1], "Value '%s' starts with a '-'")
+        check([0, 1, 3], "Value '%s' starts with a '-'")
         tp.ALLOW_DASH_AS_VALUE = True
 
         tp.ALLOW_TYPO_AS_VALUE = False
         # an option is a close match for an option, who'd guessed that...
-        t([0,2], "Value '%s' too close match to option(s) ")
+        check([0, 2, 3], "Value '%s' too close match to option(s) ")
         tp.ALLOW_TYPO_AS_VALUE = True
 
+        tp.ALLOW_OPTION_NAME_AS_VALUE = False
+        check([3], "'-%s' is a valid option")
+        tp.ALLOW_OPTION_NAME_AS_VALUE = True
 
     def test_option_as_value(self):
         """Test how valid options being used as values are handled."""
         # -s requires an argument
         self._match_testoption1_sysexit(['-b', '-s'],
                                         "-s option requires an argument")
 
-        # valid ways of specifying '-b' as a value to --store/-s
+        # only valid way of specifying '-b' as a value to --store
         topt = TestOption1(go_args=['--store=-b'])
         self.assertEqual(topt.options.store, '-b')
         self.assertFalse(topt.options.base)  # remain False (default value)
 
-        topt = TestOption1(go_args=['-s-b'])  # equivalent to --store=-b
-        self.assertEqual(topt.options.store, '-b')
-        self.assertFalse(topt.options.base)  # remain False (default value)
+        # when -b/--base is an option, the following are not accepted, since they're likely not what intended
+        self._match_testoption1_sysexit(['-sb'],
+                                        "'-b' is a valid option")
+        self._match_testoption1_sysexit(['-s', 'b'],
+                                        "'-b' is a valid option")
+        self._match_testoption1_sysexit(['--store', 'b'],
+                                        "'-b' is a valid option")
+        self._match_testoption1_sysexit(['--store', '-base'],
+                                        "'--base' is a valid option")
+        self._match_testoption1_sysexit(['-s', '-base'],
+                                        "'--base' is a valid option")
+        self._match_testoption1_sysexit(['-s-base'],
+                                        "'--base' is a valid option")
 
         # -s -b is not a valid list of flags, since it's ambiguous: is '-b' a value for '-s', or an option?
         self._match_testoption1_sysexit(['-s', '-b'],
@@ -859,7 +884,7 @@ def test_option_as_value(self):
         self._match_testoption1_sysexit(['--store', '--base'],
                                         "Value '--base' is also a valid option")
 
-        # including a non-existing option will result in it being treated as a value
+        # including a non-existing option will result in it being treated as a value, but is not allowed
         self._match_testoption1_sysexit(['--store', '-f'],
                                         "Value '-f' starts with a '-'")