Use typing.py-like annotations for command args

This means:

- An annotation like (int, str) is now typing.Union[int, str].
- utils.typing got expanded so it acts like the real typing.py, with
  issubclass() working properly with typing.Union and __union_params__
  being set.
- A literal string doesn't exist anymore as annotation, instead
  @cmdutils.argument now has a 'choices' argument which can be used like
  @cmdutils.argument('arg', choices=['val1', 'val2']).
- Argument validating/converting is now entirely handled by
  argparser.type_conv instead of relying on python's argparse, i.e.
  type/choices is now not passed to argparse anymore.

.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/

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.
 

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'),
+    @cmdutils.argument('where', choices=['prev', 'next', 'up', 'increment',
-                 tab=False, bg=False, window=False):
+                                         '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('top_navigate', metavar='ACTION',
-    @cmdutils.argument('bottom_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,
+                    top_navigate: str=None, bottom_navigate: str=None,
-                    bottom_navigate: ('next', 'increment')=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:

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):
+        assert not isinstance(key, enum)
-            return key
-        try:
         return enum[key.replace('-', '_')]
-        except KeyError:
-            raise cmdexc.ArgumentTypeError("Invalid value {}.".format(key))
 
     return _get_enum_item
 
 
-def multitype_conv(types):
+def _check_choices(param, value, choices):
-    """Function factory to get a type converter for a choice of types."""
+    if value not in choices:
-    def _convert(value):
+        expected_values = ', '.join(arg_name(val) for val in choices)
-        """Convert a value according to an iterable of possible arg types."""
+        raise cmdexc.ArgumentTypeError("{}: Invalid value {} - expected "
-        for typ in set(types):
+                                       "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):
-                if value == typ:
+        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 utils.is_enum(typ):
-                return enum_getter(typ)(value)
         elif callable(typ):
             # int, float, etc.
             if isinstance(value, typ):
                 return value
+            assert isinstance(value, str)
             try:
                 return typ(value)
             except (TypeError, ValueError):
-                    pass
+                msg = '{}: Invalid {} value {}'.format(
+                    param.name, typ.__name__, value)
+                raise cmdexc.ArgumentTypeError(msg)
         else:
-                raise ValueError("Unknown type {!r}!".format(typ))
+            raise ValueError("{}: Unknown type {!r}!".format(
-        raise cmdexc.ArgumentTypeError('Invalid value {}.'.format(value))
+                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):
+            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

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):
+        if isinstance(typ, tuple):
-            type_conv[param.name] = argparser.enum_getter(typ)
+            raise TypeError("{}: Legacy tuple type annotation!".format(
-        elif isinstance(typ, tuple):
+                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),)
+                types.append(type(param.default))
-            type_conv[param.name] = argparser.multitype_conv(typ)
+            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):
+        if typ is bool:
-            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:
             kwargs['action'] = 'store_true'
-        elif typ is not None:
+        else:
-            kwargs['type'] = typ
+            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:

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:

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 <http://www.gnu.org/licenses/>.
 
+# 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:
-
+    Union = FakeUnion
-    class TypingFake:
-
-        def __getitem__(self, _item):
-            pass
-
-    Union = TypingFake()

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.

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
 

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

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', [
+@pytest.mark.parametrize('types, value, expected', [
-    (['foo'], 'foo', True),
+    # expected: None to say it's an invalid value
-    (['bar'], 'foo', False),
+    ([Enum], Enum.foo, Enum.foo),
-    (['foo', 'bar'], 'foo', True),
+    ([Enum], 'foo', Enum.foo),
-    (['foo', int], 'foo', True),
+    ([Enum], 'foo-bar', Enum.foo_bar),
-    (['bar', int], 'foo', False),
+    ([Enum], 'blubb', None),
+    ([Enum], 'foo_bar', None),
 
-    ([Enum], Enum.foo, True),
+    ([int], 2, 2),
-    ([Enum], 'foo', True),
+    ([int], '2', 2),
-    ([Enum], 'foo-bar', True),
+    ([int], '2.5', None),
-    ([Enum], 'foo_bar', True),
+    ([int], 'foo', None),
-    ([Enum], 'blubb', False),
+    ([int, str], 'foo', 'foo'),
-
-    ([int], 2, True),
-    ([int], 2.5, True),
-    ([int], '2', True),
-    ([int], '2.5', False),
-    ([int], 'foo', False),
-    ([int], None, False),
-    ([int, str], 'foo', True),
 ])
-def test_multitype_conv(types, value, valid):
+@pytest.mark.parametrize('multi', [True, False])
-    converter = argparser.multitype_conv(types)
+def test_type_conv(types, value, expected, multi):
+    param = inspect.Parameter('foo', inspect.Parameter.POSITIONAL_ONLY)
 
-    if valid:
+    if multi:
-        converter(value)
+        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

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,34 +265,34 @@ class TestRegister:
 
     Enum = usertypes.enum('Test', ['x', 'y'])
 
-    @pytest.mark.parametrize('typ, inp, expected', [
+    @pytest.mark.parametrize('typ, inp, choices, expected', [
-        (int, '42', 42),
+        (int, '42', None, 42),
-        (int, 'x', argparser.ArgumentParserError),
+        (int, 'x', None, cmdexc.ArgumentTypeError),
-        (str, 'foo', 'foo'),
+        (str, 'foo', None, 'foo'),
 
-        ((str, int), 'foo', 'foo'),
+        (typing.Union[str, int], 'foo', None, 'foo'),
-        ((str, int), '42', 42),
+        (typing.Union[str, int], '42', None, 42),
 
-        (('foo', int), 'foo', 'foo'),
+        # Choices
-        (('foo', int), '42', 42),
+        (str, 'foo', ['foo'], 'foo'),
-        (('foo', int), 'bar', cmdexc.ArgumentTypeError),
+        (str, 'bar', ['foo'], cmdexc.ArgumentTypeError),
 
-        (Enum, 'x', Enum.x),
+        # Choices with Union: only checked when it's a str
-        (Enum, 'z', argparser.ArgumentParserError),
+        (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])
 
         if expected is cmdexc.ArgumentTypeError: