Rework the code to control via ExtOptionParser constants

lib/vsc/utils/generaloption.py

@@ -33,6 +33,7 @@
 
 import ConfigParser
 import copy
+import difflib
 import inspect
 import operator
 import os
@@ -225,29 +226,16 @@ def store_or(option, opt_str, value, parser, *args, **kwargs):
     def process(self, opt, value, values, parser):
         """Handle option-as-value issues before actually processing option."""
 
-        if hasattr(parser, 'orig_rargs') and (value in parser._long_opt or value in parser._short_opt):
-            orig_rargs = getattr(parser, 'orig_rargs')
-
-            # Check if this is a commandline issue: values that are options
-            # that are passed via --longopt=value or -sVALUE are not a problem.
-            # When processing the enviroment and/or configfile, we always set
-            # --longopt=value, so no issues there.
-
-            # index of last parsed arg in orig_rargs via remainder of parser.rargs
-            last_parsed_idx = len(orig_rargs) - len(parser.rargs) - 1
-            if orig_rargs[last_parsed_idx] == value:
-                # the value that was passed (implying the option always requires a value)
-                # is an option, and it's very ambigous.
-                if self._long_opts:
-                    force_fmt = "%s=" % self._long_opts[0]
-                else:
-                    force_fmt = "%s" % self._short_opts[0]
-
-                errmsg = "Value '%s' is also a valid option, use '%s%s' instead." % (value, force_fmt, value)
-                raise OptionValueError(errmsg)
+        if hasattr(parser, 'is_value_a_commandline_option'):
+            prefix = "%s=" % self._long_opts[0] if self._long_opts else self._short_opts[0]
+            errmsg = parser.is_value_a_commandline_option(value)
+            if errmsg is not None:
+                self.log.raiseException("%s. Use '%s%s' if the value is correct." % (errmsg, prefix, value),
+                                        exception=OptionValueError)
 
         return Option.process(self, opt, value, values, parser)
 
+
     def take_action(self, action, dest, opt, value, values, parser):
         """Extended take_action"""
         orig_action = action  # keep copy
@@ -445,6 +433,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
+
     def __init__(self, *args, **kwargs):
         """
         Following named arguments are specific to ExtOptionParser
@@ -506,6 +498,56 @@ def parse_args(self, args=None, values=None):
         # continue as usual
         return OptionParser.parse_args(self, args=args, values=values)
 
+    def is_value_a_commandline_option(self, 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.
+
+        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.
+        """
+        # Values that are/could be options that are passed via
+        # --longopt=value or -sVALUE are not a problem.
+        # When processing the enviroment and/or configfile, we always set
+        # --longopt=value, so no issues there either.
+
+        try:
+            cmdline_index = self.orig_rargs.index(value)
+        except ValueError:
+            # There is no ambiguity if the value is not passed
+            # as standalone argument via commandline
+            return None
+
+        if index is None:
+            # index of last parsed arg in orig_rargs via remainder of rargs
+            index = len(self.orig_rargs) - len(self.rargs) - 1
+
+        if 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_AS_VALUE) and (value in self._long_opt or value in self._short_opt):
+            return "Value '%s' is also a valid option" % value
+
+        if not self.ALLOW_DASH_AS_VALUE and value.startswith('-'):
+            return "Value '%s' starts with a '-'" % value
+
+        if not self.ALLOW_TYPO_AS_VALUE:
+            possibilities = self._long_opt.keys() + self._short_opt.keys()
+            # also on optionnames, i.e. without the -- / -
+            possibilities.extend([x.lstrip('-') for x in possibilities])
+            # max 3 options; minimum score is taken from EB experience
+            matches = difflib.get_close_matches(value, possibilities, 3, 0.85)
+
+            if matches:
+                return "Value '%s' too close match to option(s) %s" % (value, ', '.join(matches))
+
+        return None
+
     def set_description_docstring(self):
         """Try to find the main docstring and add it if description is not None"""
         stack = inspect.stack()[-1]

test/generaloption.py

@@ -765,20 +765,70 @@ def raise_error(msg, *args):
                               go_args=['--level-level'], envvar_prefix='GENERALOPTIONTEST', error_env_options=True,
                               error_env_option_method=raise_error)
 
-    def test_nosuchoption(self):
-        """Test catching of non-existing options."""
+    def _match_testoption1_sysexit(self, args, msg):
         self.mock_stderr(True)  # reset
-        self.assertErrorRegex(SystemExit, '.*', TestOption1, go_args=['--nosuchoptiondefinedfoobar'])
+        self.assertErrorRegex(SystemExit, '.*', TestOption1, go_args=args)
         stderr = self.get_stderr()
-        self.assertTrue("no such option: --nosuchoptiondefinedfoobar" in stderr)
+        self.assertTrue(msg in stderr)
+
+    def test_nosuchoption(self):
+        """Test catching of non-existing options."""
+        self._match_testoption1_sysexit(['--nosuchoptiondefinedfoobar'],
+                                        "no such option: --nosuchoptiondefinedfoobar")
+
+    def test_is_value_a_commandline_option(self):
+        """Test ExtOptionParser is_value_a_commandline_option method"""
+        topt = TestOption1()
+        tp = topt.parser
+        fn = tp.is_value_a_commandline_option
+
+        self.assertEqual(tp.ALLOW_OPTION_AS_VALUE, False,
+                         msg="default do not allow value as option")
+        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.orig_rargs = ['--base', '-something', 'base']
+        def t(fail, tpl):
+            for idx, value in enumerate(tp.orig_rargs):
+                msg = tpl % value
+                res = fn(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_AS_VALUE = True
+        tp.ALLOW_DASH_AS_VALUE = True
+        tp.ALLOW_TYPO_AS_VALUE = True
+
+        t([-1], 'x%sx') # all ok
+
+        tp.ALLOW_OPTION_AS_VALUE = False
+        t([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 '-'")
+        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) ")
+        tp.ALLOW_TYPO_AS_VALUE = True
+
 
     def test_option_as_value(self):
         """Test how valid options being used as values are handled."""
         # -s requires an argument
-        self.mock_stderr(True)
-        self.assertErrorRegex(SystemExit, '.*', TestOption1, go_args=['-b', '-s'])
-        stderr = self.get_stderr()
-        self.assertTrue("-s option requires an argument" in stderr)
+        self._match_testoption1_sysexit(['-b', '-s'],
+                                        "-s option requires an argument")
 
         # valid ways of specifying '-b' as a value to --store/-s
         topt = TestOption1(go_args=['--store=-b'])
@@ -790,34 +840,31 @@ def test_option_as_value(self):
         self.assertFalse(topt.options.base)  # remain False (default value)
 
         # -s -b is not a valid list of flags, since it's ambiguous: is '-b' a value for '-s', or an option?
-        self.mock_stderr(True)  # reset
-        self.assertErrorRegex(SystemExit, '.*', TestOption1, go_args=['-s', '-b'])
-        stderr = self.get_stderr()
-        self.assertTrue("Value '-b' is also a valid option" in stderr)
+        self._match_testoption1_sysexit(['-s', '-b'],
+                                        "Value '-b' is also a valid option")
 
         # same for --store -b
-        self.mock_stderr(True)  # reset
-        self.assertErrorRegex(SystemExit, '.*', TestOption1, go_args=['--store', '-b'])
-        stderr = self.get_stderr()
-        self.assertTrue("Value '-b' is also a valid option" in stderr)
+        self._match_testoption1_sysexit(['--store', '-b'],
+                                        "Value '-b' is also a valid option")
 
         # same for --store --base
-        self.mock_stderr(True)  # reset
-        self.assertErrorRegex(SystemExit, '.*', TestOption1, go_args=['--store', '--base'])
-        stderr = self.get_stderr()
-        self.assertTrue("Value '--base' is also a valid option" in stderr)
+        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
-        topt = TestOption1(go_args=['--store', '-f'])
-        self.assertEqual(topt.options.store, '-f')
-        topt = TestOption1(go_args=['--store', '--foo'])
-        self.assertEqual(topt.options.store, '--foo')
+        self._match_testoption1_sysexit(['--store', '-f'],
+                                        "Value '-f' starts with a '-'")
+
+        self._match_testoption1_sysexit(['--store', '--foo'],
+                                        "Value '--foo' starts with a '-'")
 
         # non-existing options are still reported if they're not consumed as a value
-        self.mock_stderr(True)  # reset
-        self.assertErrorRegex(SystemExit, '.*', TestOption1, go_args=['--store', '--foo', '--nosuchoptiondefinedfoobar'])
-        stderr = self.get_stderr()
-        self.assertTrue("no such option: --nosuchoptiondefinedfoobar" in stderr)
+        self._match_testoption1_sysexit(['--nosuchoptiondefinedfoobar', '--store', '--foo'],
+                                        "no such option: --nosuchoptiondefinedfoobar")
+
+        # but first error still wins
+        self._match_testoption1_sysexit(['--store', '--foo', '--nosuchoptiondefinedfoobar'],
+                                        "Value '--foo' starts with a '-'")
 
 def suite():
     """ returns all the testcases in this module """