From dc0d34bda67a94fe537fe885153a5abbf6d5883e Mon Sep 17 00:00:00 2001 From: Kenneth Hoste Date: Fri, 9 Oct 2015 20:48:02 +0200 Subject: [PATCH 1/7] fix @boegel's remarks ;-) --- test/generaloption.py | 42 ++++++++++++++++++++++++++++++------------ 1 file changed, 30 insertions(+), 12 deletions(-) diff --git a/test/generaloption.py b/test/generaloption.py index 44f02878..5504198e 100644 --- a/test/generaloption.py +++ b/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,7 +797,6 @@ 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_AS_VALUE, False, msg="default do not allow value as option") @@ -800,10 +808,11 @@ def test_is_value_a_commandline_option(self): # 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): + 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(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)) @@ -815,20 +824,20 @@ def t(fail, tpl): 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], "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], "Value '%s' too close match to option(s) ") tp.ALLOW_TYPO_AS_VALUE = True @@ -843,11 +852,16 @@ def test_option_as_value(self): 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 + topt = TestOption1(go_args=['-s-b']) # equivalent to --store=-b (even though -b is a valid short option) self.assertEqual(topt.options.store, '-b') 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? + topt = TestOption1(go_args=['-sb']) # equivalent to --store=b (even though -b is a valid short option) + self.assertEqual(topt.options.store, 'b') + self.assertFalse(topt.options.base) # remain False (default value) + + + # -s -b is not a valid list of flags (by default), since it's ambiguous: is '-b' a value for '-s', or an option? self._match_testoption1_sysexit(['-s', '-b'], "Value '-b' is also a valid option") @@ -855,11 +869,15 @@ def test_option_as_value(self): self._match_testoption1_sysexit(['--store', '-b'], "Value '-b' is also a valid option") + # same for --store=-b + self._match_testoption1_sysexit(['--store=-b'], + "Value '-b' is also a valid option") + # same for --store --base 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 (by default) self._match_testoption1_sysexit(['--store', '-f'], "Value '-f' starts with a '-'") From d3c412290969d1dd34a833b244e393f3b0c6754d Mon Sep 17 00:00:00 2001 From: Kenneth Hoste Date: Sat, 10 Oct 2015 09:33:35 +0200 Subject: [PATCH 2/7] implement support for ALLOW_OPTION_NAME_AS_VALUE + enhance tests --- lib/vsc/utils/generaloption.py | 32 ++++++++++++++++++------------ test/generaloption.py | 36 +++++++++++++++++++--------------- 2 files changed, 39 insertions(+), 29 deletions(-) diff --git a/lib/vsc/utils/generaloption.py b/lib/vsc/utils/generaloption.py index bfc53035..79e0c8d4 100644 --- a/lib/vsc/utils/generaloption.py +++ b/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,7 +489,7 @@ 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. @@ -497,33 +498,38 @@ def is_value_a_commandline_option(self, value, index=None): 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. + # --longopt=value is not a problem. # When processing the enviroment and/or configfile, we always set # --longopt=value, so no issues there either. - + 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('--'): + # --longopt=value is unambigouos + return None if index is None: # index of last parsed arg in orig_rargs 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 + if not self.ALLOW_OPTION_NAME_AS_VALUE: + if '-%s' % value in self._short_opt or '-%s' % value in self._long_opt: + return "'-%(val)s' is a valid option, so using '%(val)s' as value is likely a typo" % {'val': value} + # 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('-'): + if not self.ALLOW_DASH_AS_VALUE and value is not None and value.startswith('-'): return "Value '%s' starts with a '-'" % value if not self.ALLOW_TYPO_AS_VALUE: diff --git a/test/generaloption.py b/test/generaloption.py index 5504198e..e7ea6a4b 100644 --- a/test/generaloption.py +++ b/test/generaloption.py @@ -847,21 +847,29 @@ def test_option_as_value(self): 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 (even though -b is a valid short option) - self.assertEqual(topt.options.store, '-b') - self.assertFalse(topt.options.base) # remain False (default value) - - topt = TestOption1(go_args=['-sb']) # equivalent to --store=b (even though -b is a valid short option) - self.assertEqual(topt.options.store, 'b') - self.assertFalse(topt.options.base) # remain False (default value) - - - # -s -b is not a valid list of flags (by default), since it's ambiguous: is '-b' a value for '-s', or an option? + # 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, so using 'b' as value is likely a typo") + self._match_testoption1_sysexit(['-s', 'b'], + "'-b' is a valid option, so using 'b' as value is likely a typo") + self._match_testoption1_sysexit(['--store', 'b'], + "'-b' is a valid option, so using 'b' as value is likely a typo") + self._match_testoption1_sysexit(['--store', '-base'], + "'--base' is a valid option, so using '-base' as value is likely a typo") + self._match_testoption1_sysexit(['-s', '-base'], + "'--base' is a valid option, so using '-base' as value is likely a typo") + self._match_testoption1_sysexit(['-s-base'], + "'--base' is a valid option, so using '-base' as value is likely a typo") + + #self._match_testoption1_sysexit(['-s-b'], + # "Value '-b' is also 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'], "Value '-b' is also a valid option") @@ -869,15 +877,11 @@ def test_option_as_value(self): self._match_testoption1_sysexit(['--store', '-b'], "Value '-b' is also a valid option") - # same for --store=-b - self._match_testoption1_sysexit(['--store=-b'], - "Value '-b' is also a valid option") - # same for --store --base 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, but is not allowed (by default) + # 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 '-'") From fd57a50d3df968807117a1e09e79be8865affb06 Mon Sep 17 00:00:00 2001 From: Kenneth Hoste Date: Sat, 10 Oct 2015 09:49:01 +0200 Subject: [PATCH 3/7] fix broken tests, shortcut for non-string value in is_value_a_commandline_option --- lib/vsc/utils/generaloption.py | 7 ++++++- test/generaloption.py | 2 +- 2 files changed, 7 insertions(+), 2 deletions(-) diff --git a/lib/vsc/utils/generaloption.py b/lib/vsc/utils/generaloption.py index 79e0c8d4..c1ef6883 100644 --- a/lib/vsc/utils/generaloption.py +++ b/lib/vsc/utils/generaloption.py @@ -504,6 +504,11 @@ def is_value_a_commandline_option(self, opt, value, index=None): # --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) @@ -529,7 +534,7 @@ def is_value_a_commandline_option(self, opt, value, index=None): 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 is not None and value.startswith('-'): + if not self.ALLOW_DASH_AS_VALUE and value.startswith('-'): return "Value '%s' starts with a '-'" % value if not self.ALLOW_TYPO_AS_VALUE: diff --git a/test/generaloption.py b/test/generaloption.py index e7ea6a4b..08e424b0 100644 --- a/test/generaloption.py +++ b/test/generaloption.py @@ -812,7 +812,7 @@ 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 = tp.is_value_a_commandline_option(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)) From 08ca81c1fa9efe9896e432a2b302e00beae77300 Mon Sep 17 00:00:00 2001 From: Kenneth Hoste Date: Sat, 10 Oct 2015 12:11:43 +0200 Subject: [PATCH 4/7] fix remarks --- lib/vsc/utils/generaloption.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/lib/vsc/utils/generaloption.py b/lib/vsc/utils/generaloption.py index c1ef6883..3748cfc8 100644 --- a/lib/vsc/utils/generaloption.py +++ b/lib/vsc/utils/generaloption.py @@ -494,6 +494,7 @@ 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 @@ -501,7 +502,7 @@ def is_value_a_commandline_option(self, opt, value, index=None): 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 is 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. @@ -515,7 +516,7 @@ def is_value_a_commandline_option(self, opt, value, index=None): except ValueError: # no index found for value, so not a stand-alone value if opt.startswith('--'): - # --longopt=value is unambigouos + # only --longopt=value is unambigouos return None if index is None: From dfafd2f5b571bf102132433ebfadd47108af5a3f Mon Sep 17 00:00:00 2001 From: Kenneth Hoste Date: Sat, 10 Oct 2015 12:13:41 +0200 Subject: [PATCH 5/7] fix comment --- lib/vsc/utils/generaloption.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/vsc/utils/generaloption.py b/lib/vsc/utils/generaloption.py index 3748cfc8..b2f61c7d 100644 --- a/lib/vsc/utils/generaloption.py +++ b/lib/vsc/utils/generaloption.py @@ -520,7 +520,7 @@ def is_value_a_commandline_option(self, opt, value, index=None): 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 cmdline_index is not None and index != cmdline_index: From d843c9dd368ba85541aea68c2f6f60b7be364347 Mon Sep 17 00:00:00 2001 From: Kenneth Hoste Date: Sat, 10 Oct 2015 12:27:04 +0200 Subject: [PATCH 6/7] fix moar remarks --- lib/vsc/utils/generaloption.py | 4 ++-- test/generaloption.py | 29 ++++++++++++++++------------- 2 files changed, 18 insertions(+), 15 deletions(-) diff --git a/lib/vsc/utils/generaloption.py b/lib/vsc/utils/generaloption.py index b2f61c7d..b8ddb893 100644 --- a/lib/vsc/utils/generaloption.py +++ b/lib/vsc/utils/generaloption.py @@ -528,8 +528,8 @@ def is_value_a_commandline_option(self, opt, value, index=None): return None if not self.ALLOW_OPTION_NAME_AS_VALUE: - if '-%s' % value in self._short_opt or '-%s' % value in self._long_opt: - return "'-%(val)s' is a valid option, so using '%(val)s' as value is likely a typo" % {'val': value} + if '-%s' % value in self._short_opt.keys() + self._long_opt.keys(): + return "'-%s' is a valid option" % value # 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): diff --git a/test/generaloption.py b/test/generaloption.py index 08e424b0..76f0bb22 100644 --- a/test/generaloption.py +++ b/test/generaloption.py @@ -798,8 +798,10 @@ def test_is_value_a_commandline_option(self): topt = TestOption1(go_args=[]) tp = topt.parser + 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, @@ -807,7 +809,7 @@ def test_is_value_a_commandline_option(self): # fake commandline args # this is purely to test the function, actual usage is in test_option_as_value - tp.commandline_arguments = ['--base', '-something', 'base'] + 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): @@ -820,6 +822,7 @@ def check(fail, tpl): 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 @@ -832,14 +835,17 @@ def check(fail, tpl): tp.ALLOW_DASH_AS_VALUE = False # options also start with a - - check([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... - check([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.""" @@ -854,20 +860,17 @@ def test_option_as_value(self): # 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, so using 'b' as value is likely a typo") + "'-b' is a valid option") self._match_testoption1_sysexit(['-s', 'b'], - "'-b' is a valid option, so using 'b' as value is likely a typo") + "'-b' is a valid option") self._match_testoption1_sysexit(['--store', 'b'], - "'-b' is a valid option, so using 'b' as value is likely a typo") + "'-b' is a valid option") self._match_testoption1_sysexit(['--store', '-base'], - "'--base' is a valid option, so using '-base' as value is likely a typo") + "'--base' is a valid option") self._match_testoption1_sysexit(['-s', '-base'], - "'--base' is a valid option, so using '-base' as value is likely a typo") + "'--base' is a valid option") self._match_testoption1_sysexit(['-s-base'], - "'--base' is a valid option, so using '-base' as value is likely a typo") - - #self._match_testoption1_sysexit(['-s-b'], - # "Value '-b' is also a valid option") + "'--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'], From e5778690f1ef9672f9cd5b48cd8b414886e56463 Mon Sep 17 00:00:00 2001 From: Kenneth Hoste Date: Sat, 10 Oct 2015 12:30:24 +0200 Subject: [PATCH 7/7] fix MOAR remarks --- lib/vsc/utils/generaloption.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/vsc/utils/generaloption.py b/lib/vsc/utils/generaloption.py index b8ddb893..335e8eee 100644 --- a/lib/vsc/utils/generaloption.py +++ b/lib/vsc/utils/generaloption.py @@ -528,10 +528,10 @@ def is_value_a_commandline_option(self, opt, value, index=None): return None if not self.ALLOW_OPTION_NAME_AS_VALUE: - if '-%s' % value in self._short_opt.keys() + self._long_opt.keys(): + 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 - # 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