diff --git a/.pylintrc b/.pylintrc index 4c64f485e..f5ef697a1 100644 --- a/.pylintrc +++ b/.pylintrc @@ -57,6 +57,9 @@ dummy-variables-rgx=_.* [DESIGN] max-args=10 +[CLASSES] +valid-metaclass-classmethod-first-arg=cls + [TYPECHECK] # MsgType added as WORKAROUND for # https://bitbucket.org/logilab/pylint/issues/690/ diff --git a/doc/help/commands.asciidoc b/doc/help/commands.asciidoc index da912ed74..47232f203 100644 --- a/doc/help/commands.asciidoc +++ b/doc/help/commands.asciidoc @@ -95,7 +95,7 @@ How many pages to go back. [[bind]] === bind -Syntax: +:bind [*--mode* 'MODE'] [*--force*] 'key' ['command']+ +Syntax: +:bind [*--mode* 'mode'] [*--force*] 'key' ['command']+ Bind a key to a command. @@ -166,7 +166,7 @@ Close the current window. [[download]] === download -Syntax: +:download [*--mhtml*] [*--dest* 'DEST'] ['url'] ['dest-old']+ +Syntax: +:download [*--mhtml*] [*--dest* 'dest'] ['url'] ['dest-old']+ Download a given URL, or current page if no URL given. diff --git a/qutebrowser/browser/commands.py b/qutebrowser/browser/commands.py index abe51b55e..721603f3e 100644 --- a/qutebrowser/browser/commands.py +++ b/qutebrowser/browser/commands.py @@ -42,7 +42,7 @@ from qutebrowser.config import config, configexc from qutebrowser.browser import webelem, inspector, urlmarks, downloads, mhtml from qutebrowser.keyinput import modeman from qutebrowser.utils import (message, usertypes, log, qtutils, urlutils, - objreg, utils) + objreg, utils, typing) from qutebrowser.utils.usertypes import KeyMode from qutebrowser.misc import editor, guiprocess from qutebrowser.completion.models import instances, sortfilter @@ -455,8 +455,9 @@ class CommandDispatcher: self._open(url, tab, background, window) @cmdutils.register(instance='command-dispatcher', scope='window') - def navigate(self, where: ('prev', 'next', 'up', 'increment', 'decrement'), - tab=False, bg=False, window=False): + @cmdutils.argument('where', choices=['prev', 'next', 'up', 'increment', + 'decrement']) + def navigate(self, where: str, tab=False, bg=False, window=False): """Open typical prev/next links or navigate using the URL path. This tries to automatically click on typical _Previous Page_ or @@ -521,7 +522,7 @@ class CommandDispatcher: @cmdutils.register(instance='command-dispatcher', hide=True, scope='window') @cmdutils.argument('count', count=True) - def scroll(self, direction: (str, int), count=1): + def scroll(self, direction: typing.Union[str, int], count=1): """Scroll the current tab in the given direction. Args: @@ -618,11 +619,12 @@ class CommandDispatcher: @cmdutils.register(instance='command-dispatcher', hide=True, scope='window') @cmdutils.argument('count', count=True) - @cmdutils.argument('top_navigate', metavar='ACTION') - @cmdutils.argument('bottom_navigate', metavar='ACTION') + @cmdutils.argument('top_navigate', metavar='ACTION', + choices=('prev', 'decrement')) + @cmdutils.argument('bottom_navigate', metavar='ACTION', + choices=('next', 'increment')) def scroll_page(self, x: float, y: float, *, - top_navigate: ('prev', 'decrement')=None, - bottom_navigate: ('next', 'increment')=None, + top_navigate: str=None, bottom_navigate: str=None, count=1): """Scroll the frame page-wise. @@ -918,8 +920,9 @@ class CommandDispatcher: tabbed_browser.setCurrentIndex(idx-1) @cmdutils.register(instance='command-dispatcher', scope='window') + @cmdutils.argument('index', choices=['last']) @cmdutils.argument('count', count=True) - def tab_focus(self, index: (int, 'last')=None, count=None): + def tab_focus(self, index: typing.Union[str, int]=None, count=None): """Select the tab given as argument/[count]. If neither count nor index are given, it behaves like tab-next. @@ -951,8 +954,9 @@ class CommandDispatcher: idx)) @cmdutils.register(instance='command-dispatcher', scope='window') + @cmdutils.argument('direction', choices=['+', '-']) @cmdutils.argument('count', count=True) - def tab_move(self, direction: ('+', '-')=None, count=None): + def tab_move(self, direction: str=None, count=None): """Move the current tab. Args: diff --git a/qutebrowser/commands/argparser.py b/qutebrowser/commands/argparser.py index 4bf5fc6e0..edb6de306 100644 --- a/qutebrowser/commands/argparser.py +++ b/qutebrowser/commands/argparser.py @@ -19,7 +19,6 @@ """argparse.ArgumentParser subclass to parse qutebrowser commands.""" - import argparse from PyQt5.QtCore import QUrl @@ -84,43 +83,93 @@ class ArgumentParser(argparse.ArgumentParser): raise ArgumentParserError(msg.capitalize()) -def enum_getter(enum): +def arg_name(name): + """Get the name an argument should have based on its Python 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. """ - if isinstance(key, enum): - return key - try: - return enum[key.replace('-', '_')] - except KeyError: - raise cmdexc.ArgumentTypeError("Invalid value {}.".format(key)) + assert not isinstance(key, enum) + return enum[key.replace('-', '_')] return _get_enum_item -def multitype_conv(types): - """Function factory to get a type converter for a choice of types.""" +def _check_choices(param, value, choices): + if value not in choices: + expected_values = ', '.join(arg_name(val) for val in choices) + raise cmdexc.ArgumentTypeError("{}: Invalid value {} - expected " + "one of: {}".format( + param.name, value, expected_values)) + + +def type_conv(param, typ, str_choices=None): + """Function factory to get a type converter for a single type. + + Args: + param: The argparse.Parameter we're checking + types: The allowed type + str_choices: The allowed choices if the type ends up being a string + """ + if isinstance(typ, str): + raise TypeError("{}: Legacy string type!".format(param.name)) + + def _convert(value): + if value is param.default: + 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) + 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 + + +def multitype_conv(param, types, str_choices=None): + """Function factory to get a type converter for a choice of types. + + Args: + param: The inspect.Parameter we're checking + types: The allowed types ("overloads") + str_choices: The allowed choices if the type ends up being a string + """ def _convert(value): """Convert a value according to an iterable of possible arg types.""" for typ in set(types): - if isinstance(typ, str): - if value == typ: - return value - elif utils.is_enum(typ): - return enum_getter(typ)(value) - elif callable(typ): - # int, float, etc. - if isinstance(value, typ): - return value - try: - return typ(value) - except (TypeError, ValueError): - pass - else: - raise ValueError("Unknown type {!r}!".format(typ)) - raise cmdexc.ArgumentTypeError('Invalid value {}.'.format(value)) + 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 diff --git a/qutebrowser/commands/command.py b/qutebrowser/commands/command.py index 621ef060b..02fa17c66 100644 --- a/qutebrowser/commands/command.py +++ b/qutebrowser/commands/command.py @@ -26,21 +26,17 @@ import traceback from PyQt5.QtWebKit import QWebSettings from qutebrowser.commands import cmdexc, argparser -from qutebrowser.utils import log, utils, message, docutils, objreg, usertypes +from qutebrowser.utils import (log, utils, message, docutils, objreg, + usertypes, typing) from qutebrowser.utils import debug as debug_utils -def arg_name(name): - """Get the name an argument should have based on its Python name.""" - return name.rstrip('_').replace('_', '-') - - class ArgInfo: """Information about an argument.""" def __init__(self, win_id=False, count=False, flag=None, hide=False, - metavar=None, completion=None): + metavar=None, completion=None, choices=None): if win_id and count: raise TypeError("Argument marked as both count/win_id!") self.win_id = win_id @@ -49,6 +45,7 @@ class ArgInfo: self.hide = hide self.metavar = metavar self.completion = completion + self.choices = choices def __eq__(self, other): return (self.win_id == other.win_id and @@ -56,13 +53,14 @@ class ArgInfo: self.flag == other.flag and self.hide == other.hide and self.metavar == other.metavar and - self.completion == other.completion) + self.completion == other.completion and + self.choices == other.choices) def __repr__(self): return utils.get_repr(self, win_id=self.win_id, count=self.count, flag=self.flag, hide=self.hide, metavar=self.metavar, completion=self.completion, - constructor=True) + choices=self.choices, constructor=True) class Command: @@ -208,12 +206,25 @@ class Command: typ: The type of the parameter. """ type_conv = {} - if utils.is_enum(typ): - type_conv[param.name] = argparser.enum_getter(typ) - elif isinstance(typ, tuple): + 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: - typ = typ + (type(param.default),) - type_conv[param.name] = argparser.multitype_conv(typ) + 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): @@ -289,15 +300,13 @@ class Command: arg_info = self.get_arg_info(param) - if isinstance(typ, tuple): - kwargs['metavar'] = arg_info.metavar or param.name - elif utils.is_enum(typ): - kwargs['choices'] = [arg_name(e.name) for e in typ] - kwargs['metavar'] = arg_info.metavar or param.name - elif typ is bool: + if typ is bool: kwargs['action'] = 'store_true' - elif typ is not None: - kwargs['type'] = typ + else: + if arg_info.metavar is not None: + kwargs['metavar'] = arg_info.metavar + else: + kwargs['metavar'] = argparser.arg_name(param.name) if param.kind == inspect.Parameter.VAR_POSITIONAL: kwargs['nargs'] = '*' if self._star_args_optional else '+' @@ -319,7 +328,7 @@ class Command: A list of args. """ args = [] - name = arg_name(param.name) + name = argparser.arg_name(param.name) arg_info = self.get_arg_info(param) if arg_info.flag is not None: diff --git a/qutebrowser/misc/utilcmds.py b/qutebrowser/misc/utilcmds.py index 5025a2c41..b4fe9e764 100644 --- a/qutebrowser/misc/utilcmds.py +++ b/qutebrowser/misc/utilcmds.py @@ -119,7 +119,8 @@ def message_warning(win_id, text): @cmdutils.register(debug=True) -def debug_crash(typ: ('exception', 'segfault')='exception'): +@cmdutils.argument('typ', choices=['exception', 'segfault']) +def debug_crash(typ='exception'): """Crash for debugging purposes. Args: diff --git a/qutebrowser/utils/typing.py b/qutebrowser/utils/typing.py index 04df10059..87d741a6e 100644 --- a/qutebrowser/utils/typing.py +++ b/qutebrowser/utils/typing.py @@ -17,6 +17,8 @@ # You should have received a copy of the GNU General Public License # along with qutebrowser. If not, see . +# pylint: disable=unused-import,wrong-import-position,bad-mcs-method-argument + """Wrapper for Python 3.5's typing module. This wrapper is needed as both Python 3.5 and typing for PyPI isn't commonly @@ -25,13 +27,45 @@ runtime, we instead mock the typing classes (using objects to make things easier) so the typing module isn't a hard dependency. """ +# Those are defined here to make them testable easily + + +class FakeTypingMeta(type): + + """Fake typing metaclass like typing.TypingMeta.""" + + def __init__(self, *args, **kwds): # pylint: disable=super-init-not-called + pass + + def __subclasscheck__(self, cls): + """We implement this for qutebrowser.commands.command to work.""" + return isinstance(cls, FakeTypingMeta) + + +class FakeUnionMeta(FakeTypingMeta): + + """Fake union metaclass metaclass like typing.UnionMeta.""" + + def __new__(cls, name, bases, namespace, parameters=None): + if parameters is None: + return super().__new__(cls, name, bases, namespace) + self = super().__new__(cls, name, bases, {}) + self.__union_params__ = tuple(parameters) + return self + + def __getitem__(self, parameters): + return self.__class__(self.__name__, self.__bases__, + dict(self.__dict__), parameters=parameters) + + +class FakeUnion(metaclass=FakeUnionMeta): + + """Fake Union type like typing.Union.""" + + __union_params__ = None + + try: from typing import Union except ImportError: - - class TypingFake: - - def __getitem__(self, _item): - pass - - Union = TypingFake() + Union = FakeUnion diff --git a/scripts/dev/src2asciidoc.py b/scripts/dev/src2asciidoc.py index 1eaaf1192..6f8037cb9 100755 --- a/scripts/dev/src2asciidoc.py +++ b/scripts/dev/src2asciidoc.py @@ -38,7 +38,7 @@ sys.path.insert(0, os.path.join(os.path.dirname(__file__), os.pardir, import qutebrowser.app from scripts import asciidoc2html, utils from qutebrowser import qutebrowser -from qutebrowser.commands import cmdutils, command +from qutebrowser.commands import cmdutils, argparser from qutebrowser.config import configdata from qutebrowser.utils import docutils @@ -57,11 +57,11 @@ class UsageFormatter(argparse.HelpFormatter): def _get_default_metavar_for_optional(self, action): """Do name transforming when getting metavar.""" - return command.arg_name(action.dest.upper()) + return argparser.arg_name(action.dest.upper()) def _get_default_metavar_for_positional(self, action): """Do name transforming when getting metavar.""" - return command.arg_name(action.dest) + return argparser.arg_name(action.dest) def _metavar_formatter(self, action, default_metavar): """Override _metavar_formatter to add asciidoc markup to metavars. diff --git a/tests/integration/features/navigate.feature b/tests/integration/features/navigate.feature index b356e399c..e817669d4 100644 --- a/tests/integration/features/navigate.feature +++ b/tests/integration/features/navigate.feature @@ -2,7 +2,7 @@ Feature: Using :navigate Scenario: :navigate with invalid argument When I run :navigate foo - Then the error "Invalid value foo." should be shown + Then the error "where: Invalid value foo - expected one of: prev, next, up, increment, decrement" should be shown # up diff --git a/tests/integration/features/scroll.feature b/tests/integration/features/scroll.feature index 0022ce96b..676cd0890 100644 --- a/tests/integration/features/scroll.feature +++ b/tests/integration/features/scroll.feature @@ -49,7 +49,7 @@ Feature: Scrolling Scenario: :scroll-px with floats # This used to be allowed, but doesn't make much sense. When I run :scroll-px 2.5 2.5 - Then the error "scroll-px: Argument dx: invalid int value: '2.5'" should be shown + Then the error "dx: Invalid int value 2.5" should be shown And the page should not be scrolled ## :scroll diff --git a/tests/unit/commands/test_argparser.py b/tests/unit/commands/test_argparser.py index 8ffd62cfb..3eb8a6d33 100644 --- a/tests/unit/commands/test_argparser.py +++ b/tests/unit/commands/test_argparser.py @@ -19,6 +19,8 @@ """Tests for qutebrowser.commands.argparser.""" +import inspect + import pytest from PyQt5.QtCore import QUrl @@ -77,40 +79,89 @@ class TestArgumentParser: assert tabbed_browser.opened_url == expected_url -@pytest.mark.parametrize('types, value, valid', [ - (['foo'], 'foo', True), - (['bar'], 'foo', False), - (['foo', 'bar'], 'foo', True), - (['foo', int], 'foo', True), - (['bar', int], 'foo', False), +@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), - ([Enum], Enum.foo, True), - ([Enum], 'foo', True), - ([Enum], 'foo-bar', True), - ([Enum], 'foo_bar', True), - ([Enum], 'blubb', False), - - ([int], 2, True), - ([int], 2.5, True), - ([int], '2', True), - ([int], '2.5', False), - ([int], 'foo', False), - ([int], None, False), - ([int, str], 'foo', True), + ([int], 2, 2), + ([int], '2', 2), + ([int], '2.5', None), + ([int], 'foo', None), + ([int, str], 'foo', 'foo'), ]) -def test_multitype_conv(types, value, valid): - converter = argparser.multitype_conv(types) +@pytest.mark.parametrize('multi', [True, False]) +def test_type_conv(types, value, expected, multi): + param = inspect.Parameter('foo', inspect.Parameter.POSITIONAL_ONLY) - if valid: - converter(value) + if multi: + converter = argparser.multitype_conv(param, types) + elif len(types) == 1: + converter = argparser.type_conv(param, types[0]) + else: + return + + if expected is not None: + assert converter(value) == expected else: with pytest.raises(cmdexc.ArgumentTypeError) as excinfo: converter(value) - assert str(excinfo.value) == 'Invalid value {}.'.format(value) + + 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)) + else: + msg = 'foo: Invalid {} value {}'.format(types[0].__name__, value) + assert str(excinfo.value) == msg def test_multitype_conv_invalid_type(): - converter = argparser.multitype_conv([None]) + """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('') - assert str(excinfo.value) == "Unknown type None!" + assert str(excinfo.value) == "foo: Unknown type None!" + + +def test_conv_default_param(): + """The default value should always be a valid choice.""" + 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 + + +def test_conv_str_type(): + """Using a str literal as type used to mean exactly that's a valid value. + + This got replaced by @cmdutils.argument(..., choices=...), so we make sure + no string annotations are there anymore. + """ + param = inspect.Parameter('foo', inspect.Parameter.POSITIONAL_ONLY) + with pytest.raises(TypeError) as excinfo: + argparser.type_conv(param, 'val') + 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' + + +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') + msg = 'foo: Invalid value val3 - expected one of: val1, val2' + assert str(excinfo.value) == msg diff --git a/tests/unit/commands/test_cmdutils.py b/tests/unit/commands/test_cmdutils.py index ed8bcb75a..757bc2061 100644 --- a/tests/unit/commands/test_cmdutils.py +++ b/tests/unit/commands/test_cmdutils.py @@ -22,7 +22,7 @@ import pytest from qutebrowser.commands import cmdutils, cmdexc, argparser, command -from qutebrowser.utils import usertypes +from qutebrowser.utils import usertypes, typing @pytest.fixture(autouse=True) @@ -265,35 +265,35 @@ class TestRegister: Enum = usertypes.enum('Test', ['x', 'y']) - @pytest.mark.parametrize('typ, inp, expected', [ - (int, '42', 42), - (int, 'x', argparser.ArgumentParserError), - (str, 'foo', 'foo'), + @pytest.mark.parametrize('typ, inp, choices, expected', [ + (int, '42', None, 42), + (int, 'x', None, cmdexc.ArgumentTypeError), + (str, 'foo', None, 'foo'), - ((str, int), 'foo', 'foo'), - ((str, int), '42', 42), + (typing.Union[str, int], 'foo', None, 'foo'), + (typing.Union[str, int], '42', None, 42), - (('foo', int), 'foo', 'foo'), - (('foo', int), '42', 42), - (('foo', int), 'bar', cmdexc.ArgumentTypeError), + # Choices + (str, 'foo', ['foo'], 'foo'), + (str, 'bar', ['foo'], cmdexc.ArgumentTypeError), - (Enum, 'x', Enum.x), - (Enum, 'z', argparser.ArgumentParserError), + # Choices with Union: only checked when it's a str + (typing.Union[str, int], 'foo', ['foo'], 'foo'), + (typing.Union[str, int], 'bar', ['foo'], cmdexc.ArgumentTypeError), + (typing.Union[str, int], '42', ['foo'], 42), + + (Enum, 'x', None, Enum.x), + (Enum, 'z', None, cmdexc.ArgumentTypeError), ]) - def test_typed_args(self, typ, inp, expected): + def test_typed_args(self, typ, inp, choices, expected): @cmdutils.register() + @cmdutils.argument('arg', choices=choices) def fun(arg: typ): """Blah.""" pass cmd = cmdutils.cmd_dict['fun'] - - if expected is argparser.ArgumentParserError: - with pytest.raises(argparser.ArgumentParserError): - cmd.parser.parse_args([inp]) - return - else: - cmd.namespace = cmd.parser.parse_args([inp]) + cmd.namespace = cmd.parser.parse_args([inp]) if expected is cmdexc.ArgumentTypeError: with pytest.raises(cmdexc.ArgumentTypeError):