Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Generaloption: handle option-as-value #185

Merged
merged 21 commits into from
Oct 10, 2015
Merged

Generaloption: handle option-as-value #185

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
4a01bde
Initial attempt at fixing the option-as-value issue
stdweird Aug 18, 2015
80546cd
add extra unit test for option-as-value
boegel Aug 18, 2015
27b3a0e
--amend
boegel Aug 18, 2015
c012cf1
add tests to check for 'no such option' errors
boegel Aug 18, 2015
3c37349
correct test where non-existing option is consumed as a value
boegel Aug 18, 2015
94702c8
style fixes
boegel Aug 18, 2015
7e4cb8c
--amend
boegel Aug 18, 2015
c9d9860
--amend
boegel Aug 18, 2015
9ce069f
make sure environment options aren't processed multiple times (via _g…
boegel Aug 18, 2015
1210c40
not ... is None <<< is not None
boegel Aug 18, 2015
a457921
Rework the code to control via ExtOptionParser constants
stdweird Sep 10, 2015
a53ef03
no need for orig_rargs, can use commandline_arguments (this avoids al…
stdweird Oct 8, 2015
850c8e3
bump version to 2.4.1
stdweird Oct 9, 2015
dc0d34b
fix @boegel's remarks ;-)
boegel Oct 9, 2015
d3c4122
implement support for ALLOW_OPTION_NAME_AS_VALUE + enhance tests
boegel Oct 10, 2015
fd57a50
fix broken tests, shortcut for non-string value in is_value_a_command…
boegel Oct 10, 2015
08ca81c
fix remarks
boegel Oct 10, 2015
dfafd2f
fix comment
boegel Oct 10, 2015
d843c9d
fix moar remarks
boegel Oct 10, 2015
e577869
fix MOAR remarks
boegel Oct 10, 2015
1a4093b
Merge pull request #10 from boegel/go_nooptionasvalue
stdweird Oct 10, 2015
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
79 changes: 79 additions & 0 deletions lib/vsc/utils/generaloption.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@

import ConfigParser
import copy
import difflib
import inspect
import operator
import os
Expand Down Expand Up @@ -222,6 +223,18 @@ def store_or(option, opt_str, value, parser, *args, **kwargs):
else:
self.log.raiseException("_set_attrs: unknown store_or %s" % self.store_or, exception=ValueError)

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(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),
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
Expand Down Expand Up @@ -419,6 +432,11 @@ class ExtOptionParser(OptionParser):
VALUES_CLASS = Values
DESCRIPTION_DOCSTRING = False

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):
"""
Following named arguments are specific to ExtOptionParser
Expand Down Expand Up @@ -471,6 +489,67 @@ def __init__(self, *args, **kwargs):
self.environment_arguments = None
self.commandline_arguments = 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; --longopt=value is never considered ambiguous, regardless of the value.
"""
# Values that are/could be options that are passed via
# 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:
# 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 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:
# This is not the value you are looking for
return None

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

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]
Expand Down
2 changes: 1 addition & 1 deletion setup.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ def remove_bdist_rpm_source_file():

PACKAGE = {
'name': 'vsc-base',
'version': '2.4.0',
'version': '2.4.1',
'author': [sdw, jt, ag, kh],
'maintainer': [sdw, jt, ag, kh],
'packages': ['vsc', 'vsc.install', 'vsc.utils'],
Expand Down
160 changes: 147 additions & 13 deletions test/generaloption.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ def base_options(self):
"base":("Long and short base option", None, "store_true", False, 'b'),
"longbase":("Long-only base option", None, "store_true", True),
"justatest":("Another long based option", None, "store_true", True),
"store":("Store option", None, "store", None),
"store":("Store option", None, "store", None, 's'),
"store-with-dash":("Store option with dash in name", None, "store", None),
"aregexopt":("A regex option", None, "regex", None),
}
Expand Down Expand Up @@ -613,21 +613,29 @@ def test_get_options_by_property(self):

def test_loglevel(self):
"""Test the loglevel default setting"""
def _loglevel(lvl, msg):
lvl_int = topt.log.getEffectiveLevel()
lvl_name = [k for k,v in logging._levelNames.items() if v == lvl_int][0]
self.assertEqual(lvl_int,
fancylogger.getLevelInt(lvl),
msg="%s (expected %s got %s)" % (msg, lvl, lvl_name))


topt = TestOption1(go_args=['--ext-optional=REALVALUE'], go_nosystemexit=True,)
self.assertEqual(topt.log.getEffectiveLevel(), fancylogger.getLevelInt(topt.DEFAULT_LOGLEVEL.upper()))
_loglevel(topt.DEFAULT_LOGLEVEL.upper(), 'Test default loglevel')

topt = TestOption1(go_args=['--debug'], go_nosystemexit=True,)
self.assertEqual(topt.log.getEffectiveLevel(), logging.DEBUG)
_loglevel('DEBUG', '--debug gives DEBUG')

topt = TestOption1(go_args=['--info'], go_nosystemexit=True,)
self.assertEqual(topt.log.getEffectiveLevel(), logging.INFO)
_loglevel('INFO', '--info gives INFO')

topt = TestOption1(go_args=['--quiet'], go_nosystemexit=True,)
self.assertEqual(topt.log.getEffectiveLevel(), logging.WARNING)
_loglevel('WARNING', '--quiet gives WARNING')

# last one wins
topt = TestOption1(go_args=['--debug', '--info', '--quiet'], go_nosystemexit=True,)
self.assertEqual(topt.log.getEffectiveLevel(), logging.WARNING)
_loglevel('WARNING', 'last wins: --debug --info --quiet gives WARNING')

CONFIGFILE1 = """
[base]
Expand All @@ -642,7 +650,7 @@ def test_loglevel(self):
go_nosystemexit=True,
envvar_prefix=envvar
)
self.assertEqual(topt.log.getEffectiveLevel(), logging.DEBUG)
_loglevel('DEBUG', 'DEBUG set via configfile')

# set via environment; environment wins over cfg file
os.environ['%s_INFO' % envvar] = '1';
Expand All @@ -651,15 +659,15 @@ def test_loglevel(self):
go_nosystemexit=True,
envvar_prefix=envvar
)
self.assertEqual(topt.log.getEffectiveLevel(), logging.INFO)
_loglevel('INFO', 'env wins: debug in configfile and _INFO in env gives INFO')

# commandline always wins
topt = TestOption1(go_configfiles=[tmp1.name],
go_args=['--quiet'],
go_nosystemexit=True,
envvar_prefix=envvar
)
self.assertEqual(topt.log.getEffectiveLevel(), logging.WARNING)
_loglevel('WARNING', 'commandline wins: debug in configfile, _INFO in env and --quiet gives WARNING')

# remove tmp1
del os.environ['%s_INFO' % envvar]
Expand Down Expand Up @@ -748,12 +756,13 @@ def test_error_env_options(self):

os.environ['GENERALOPTIONTEST_XYZ'] = '1'
topt1 = TestOption1(go_args=['--level-level'], envvar_prefix='GENERALOPTIONTEST')
# no errors logged
self.assertEqual(self.count_logcache('error'), 0)
self.assertEqual(self.count_logcache('error'), 0,
msg='no errors logged, got %s' % self.count_logcache('error'))

topt1 = TestOption1(go_args=['--level-level'], envvar_prefix='GENERALOPTIONTEST', error_env_options=True)
# one error should be logged
self.assertEqual(self.count_logcache('error'), 1)
print self.LOGCACHE['error']
self.assertEqual(self.count_logcache('error'), 1,
msg='one error should be logged, got %s' % self.count_logcache('error'))

# using a custom error method
def raise_error(msg, *args):
Expand All @@ -764,6 +773,131 @@ def raise_error(msg, *args):
go_args=['--level-level'], envvar_prefix='GENERALOPTIONTEST', error_env_options=True,
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
# 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)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

add self.mock_stderr(False) here to disable the mocking (so we can actually see something if something is broken hard :)

maybe the self.assertErrorRegex during mocking isn't a good idea either, will we see the actual failed test since stderr is being mocked?

maybe this is a good place for try/except/finally instead of self.assertErrorRegex:

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
    # purposely not using self.assertErrorRegex, since we're mocking stderr...
    try:
        TestOption(go_args=args)
    except SytemExit:
        system_exit = True
    finally:
        self.mock_stderr(False)
    self.assertTrue(system_exit)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah, yes, and add the self.assertTrue(msg in stderr) too

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i don't understand. isn't the setUp / tearDown supposed to do this? also don't understand the reamkr wrt exceptions and stderr.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While you're mocking stderr, you won't get any output on stderr anymore.

The tearDown will disable the mocking, sure, but it's cleaner to do that in here (since you may be calling this method several times in a single test).


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(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 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', '-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 = 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

check([], 'x%sx') # all ok

tp.ALLOW_OPTION_AS_VALUE = False
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 -
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, 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")

# 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)

# 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'],
"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
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._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 """
Expand Down