From a83bf9c3eeaf9e0e7f0eeac0246dcb4e0994f6ea Mon Sep 17 00:00:00 2001 From: Florian Bruhin Date: Tue, 17 May 2016 18:53:48 +0200 Subject: [PATCH] Simplify argparser.type_conv Since we're not using those functions as argparse callbacks anymore, we can write a normal function instead of factories, which simplifies things a lot. --- qutebrowser/commands/argparser.py | 102 +++++++++++--------------- qutebrowser/commands/command.py | 59 ++++++--------- tests/unit/commands/test_argparser.py | 63 ++++++++-------- 3 files changed, 97 insertions(+), 127 deletions(-) diff --git a/qutebrowser/commands/argparser.py b/qutebrowser/commands/argparser.py index fe21a5373..a0ede657c 100644 --- a/qutebrowser/commands/argparser.py +++ b/qutebrowser/commands/argparser.py @@ -88,19 +88,6 @@ def arg_name(name): return name.rstrip('_').replace('_', '-') -def _enum_getter(enum): - """Function factory to get an enum getter.""" - def _get_enum_item(key): - """Helper function to get an enum item. - - Passes through existing items unmodified. - """ - assert not isinstance(key, enum) - return enum[key.replace('-', '_')] - - return _get_enum_item - - def _check_choices(param, value, choices): if value not in choices: expected_values = ', '.join(arg_name(val) for val in choices) @@ -109,57 +96,61 @@ def _check_choices(param, value, choices): param.name, value, expected_values)) -def type_conv(param, typ, str_choices=None): - """Function factory to get a type converter for a single type. +def type_conv(param, typ, value, *, str_choices=None): + """Convert a value based on a type. Args: param: The argparse.Parameter we're checking types: The allowed type + value: The value to convert str_choices: The allowed choices if the type ends up being a string + + Return: + The converted value """ if isinstance(typ, str): raise TypeError("{}: Legacy string type!".format(param.name)) - def _convert(value): - if value is param.default: + if value is param.default: + return value + + if utils.is_enum(typ): + if isinstance(value, typ): return value - - if utils.is_enum(typ): - if isinstance(value, typ): - return value - assert isinstance(value, str) - _check_choices(param, value, [arg_name(e.name) for e in typ]) - getter = _enum_getter(typ) - return getter(value) - elif typ is str: - assert isinstance(value, str) - if str_choices is not None: - _check_choices(param, value, str_choices) + assert isinstance(value, str) + _check_choices(param, value, [arg_name(e.name) for e in typ]) + return typ[value.replace('-', '_')] + elif typ is str: + assert isinstance(value, str) + if str_choices is not None: + _check_choices(param, value, str_choices) + return value + elif callable(typ): + # int, float, etc. + if isinstance(value, typ): return value - elif callable(typ): - # int, float, etc. - if isinstance(value, typ): - return value - assert isinstance(value, str) - try: - return typ(value) - except (TypeError, ValueError): - msg = '{}: Invalid {} value {}'.format( - param.name, typ.__name__, value) - raise cmdexc.ArgumentTypeError(msg) - else: - raise ValueError("{}: Unknown type {!r}!".format( - param.name, typ)) - return _convert + assert isinstance(value, str) + try: + return typ(value) + except (TypeError, ValueError): + msg = '{}: Invalid {} value {}'.format( + param.name, typ.__name__, value) + raise cmdexc.ArgumentTypeError(msg) + else: + raise ValueError("{}: Unknown type {!r}!".format(param.name, typ)) -def multitype_conv(param, types, str_choices=None): - """Function factory to get a type converter for a choice of types. +def multitype_conv(param, types, value, *, str_choices=None): + """Convert a value based on a choice of types. Args: param: The inspect.Parameter we're checking types: The allowed types ("overloads") + value: The value to convert str_choices: The allowed choices if the type ends up being a string + + Return: + The converted value """ types = list(set(types)) if str in types: @@ -168,15 +159,10 @@ def multitype_conv(param, types, str_choices=None): types.remove(str) types.append(str) - def _convert(value): - """Convert a value according to an iterable of possible arg types.""" - for typ in types: - try: - converter = type_conv(param, typ, str_choices) - return converter(value) - except cmdexc.ArgumentTypeError: - pass - raise cmdexc.ArgumentTypeError('{}: Invalid value {}'.format( - param.name, value)) - - return _convert + for typ in types: + try: + return type_conv(param, typ, value, str_choices=str_choices) + except cmdexc.ArgumentTypeError: + pass + raise cmdexc.ArgumentTypeError('{}: Invalid value {}'.format( + param.name, value)) diff --git a/qutebrowser/commands/command.py b/qutebrowser/commands/command.py index 02fa17c66..b7da3a98f 100644 --- a/qutebrowser/commands/command.py +++ b/qutebrowser/commands/command.py @@ -81,7 +81,6 @@ class Command: flags_with_args: A list of flags which take an argument. no_cmd_split: If true, ';;' to split sub-commands is ignored. _qute_args: The saved data from @cmdutils.argument - _type_conv: A mapping of conversion functions for arguments. _needs_js: Whether the command needs javascript enabled _modes: The modes the command can be executed in. _not_modes: The modes the command can not be executed in. @@ -138,7 +137,6 @@ class Command: self.pos_args = [] self.desc = None self.flags_with_args = [] - self._type_conv = {} # This is checked by future @cmdutils.argument calls so they fail # (as they'd be silently ignored otherwise) @@ -198,35 +196,6 @@ class Command: """Get an ArgInfo tuple for the given inspect.Parameter.""" return self._qute_args.get(param.name, ArgInfo()) - def _get_typeconv(self, param, typ): - """Get a dict with a type conversion for the parameter. - - Args: - param: The inspect.Parameter to handle. - typ: The type of the parameter. - """ - type_conv = {} - if isinstance(typ, tuple): - raise TypeError("{}: Legacy tuple type annotation!".format( - self.name)) - elif issubclass(typ, typing.Union): - # this is... slightly evil, I know - types = list(typ.__union_params__) - if param.default is not inspect.Parameter.empty: - types.append(type(param.default)) - choices = self.get_arg_info(param).choices - type_conv[param.name] = argparser.multitype_conv( - param, types, str_choices=choices) - elif typ is str: - choices = self.get_arg_info(param).choices - type_conv[param.name] = argparser.type_conv(param, typ, - str_choices=choices) - elif typ is None: - pass - else: - type_conv[param.name] = argparser.type_conv(param, typ) - return type_conv - def _inspect_special_param(self, param): """Check if the given parameter is a special one. @@ -270,7 +239,6 @@ class Command: typ = self._get_type(param) kwargs = self._param_to_argparse_kwargs(param, typ) args = self._param_to_argparse_args(param, typ) - self._type_conv.update(self._get_typeconv(param, typ)) callsig = debug_utils.format_call( self.parser.add_argument, args, kwargs, full=False) @@ -428,12 +396,27 @@ class Command: def _get_param_value(self, param): """Get the converted value for an inspect.Parameter.""" value = getattr(self.namespace, param.name) - if param.name in self._type_conv: - # We convert enum types after getting the values from - # argparse, because argparse's choices argument is - # processed after type conversation, which is not what we - # want. - value = self._type_conv[param.name](value) + typ = self._get_type(param) + + if isinstance(typ, tuple): + raise TypeError("{}: Legacy tuple type annotation!".format( + self.name)) + elif issubclass(typ, typing.Union): + # this is... slightly evil, I know + types = list(typ.__union_params__) + if param.default is not inspect.Parameter.empty: + types.append(type(param.default)) + choices = self.get_arg_info(param).choices + value = argparser.multitype_conv(param, types, value, + str_choices=choices) + elif typ is str: + choices = self.get_arg_info(param).choices + value = argparser.type_conv(param, typ, value, str_choices=choices) + elif typ is None: + pass + else: + value = argparser.type_conv(param, typ, value) + return value def _get_call_args(self, win_id): diff --git a/tests/unit/commands/test_argparser.py b/tests/unit/commands/test_argparser.py index dc39f631b..54ab31f52 100644 --- a/tests/unit/commands/test_argparser.py +++ b/tests/unit/commands/test_argparser.py @@ -80,53 +80,55 @@ class TestArgumentParser: @pytest.mark.parametrize('types, value, expected', [ - # expected: None to say it's an invalid value ([Enum], Enum.foo, Enum.foo), ([Enum], 'foo', Enum.foo), ([Enum], 'foo-bar', Enum.foo_bar), - ([Enum], 'blubb', None), - ([Enum], 'foo_bar', None), ([int], 2, 2), ([int], '2', 2), - ([int], '2.5', None), - ([int], 'foo', None), ([int, str], 'foo', 'foo'), - ([str, int], '23', 23), ]) @pytest.mark.parametrize('multi', [True, False]) -def test_type_conv(types, value, expected, multi): +def test_type_conv_valid(types, value, expected, multi): param = inspect.Parameter('foo', inspect.Parameter.POSITIONAL_ONLY) if multi: - converter = argparser.multitype_conv(param, types) + assert argparser.multitype_conv(param, types, value) == expected elif len(types) == 1: - converter = argparser.type_conv(param, types[0]) - else: - return + assert argparser.type_conv(param, types[0], value) == expected - if expected is not None: - assert converter(value) == expected - else: - with pytest.raises(cmdexc.ArgumentTypeError) as excinfo: - converter(value) +@pytest.mark.parametrize('typ, value', [ + (Enum, 'blubb'), + (Enum, 'foo_bar'), + (int, '2.5'), + (int, 'foo'), +]) +@pytest.mark.parametrize('multi', [True, False]) +def test_type_conv_invalid(typ, value, multi): + param = inspect.Parameter('foo', inspect.Parameter.POSITIONAL_ONLY) + + with pytest.raises(cmdexc.ArgumentTypeError) as excinfo: if multi: - msg = 'foo: Invalid value {}'.format(value) - elif types[0] is Enum: - msg = ('foo: Invalid value {} - expected one of: foo, ' - 'foo-bar'.format(value)) + argparser.multitype_conv(param, [typ], value) else: - msg = 'foo: Invalid {} value {}'.format(types[0].__name__, value) - assert str(excinfo.value) == msg + argparser.type_conv(param, typ, value) + + if multi: + msg = 'foo: Invalid value {}'.format(value) + elif typ is Enum: + msg = ('foo: Invalid value {} - expected one of: foo, ' + 'foo-bar'.format(value)) + else: + msg = 'foo: Invalid {} value {}'.format(typ.__name__, value) + assert str(excinfo.value) == msg def test_multitype_conv_invalid_type(): """Test using an invalid type with a multitype converter.""" param = inspect.Parameter('foo', inspect.Parameter.POSITIONAL_ONLY) - converter = argparser.multitype_conv(param, [None]) with pytest.raises(ValueError) as excinfo: - converter('') + argparser.multitype_conv(param, [None], '') assert str(excinfo.value) == "foo: Unknown type None!" @@ -135,8 +137,7 @@ def test_conv_default_param(): def func(foo=None): pass param = inspect.signature(func).parameters['foo'] - converter = argparser.type_conv(param, str, str_choices=['val']) - assert converter(None) is None + assert argparser.type_conv(param, str, None, str_choices=['val']) is None def test_conv_str_type(): @@ -147,22 +148,22 @@ def test_conv_str_type(): """ param = inspect.Parameter('foo', inspect.Parameter.POSITIONAL_ONLY) with pytest.raises(TypeError) as excinfo: - argparser.type_conv(param, 'val') + argparser.type_conv(param, 'val', None) assert str(excinfo.value) == 'foo: Legacy string type!' def test_conv_str_choices_valid(): """Calling str type with str_choices and valid value.""" param = inspect.Parameter('foo', inspect.Parameter.POSITIONAL_ONLY) - converter = argparser.type_conv(param, str, str_choices=['val1', 'val2']) - assert converter('val1') == 'val1' + converted = argparser.type_conv(param, str, 'val1', + str_choices=['val1', 'val2']) + assert converted == 'val1' def test_conv_str_choices_invalid(): """Calling str type with str_choices and invalid value.""" param = inspect.Parameter('foo', inspect.Parameter.POSITIONAL_ONLY) - converter = argparser.type_conv(param, str, str_choices=['val1', 'val2']) with pytest.raises(cmdexc.ArgumentTypeError) as excinfo: - converter('val3') + argparser.type_conv(param, str, 'val3', str_choices=['val1', 'val2']) msg = 'foo: Invalid value val3 - expected one of: val1, val2' assert str(excinfo.value) == msg