From 0e771db7f1788b778bd0c51e1f67c7b31851e987 Mon Sep 17 00:00:00 2001 From: Florian Bruhin Date: Thu, 9 Oct 2014 06:33:24 +0200 Subject: [PATCH] Use annotation instead of special argument names. Explicit is better than implicit. Fixes #161. --- doc/HACKING.asciidoc | 3 -- qutebrowser/browser/commands.py | 44 ++++++++++------- qutebrowser/browser/downloads.py | 2 +- qutebrowser/browser/quickmarks.py | 2 +- qutebrowser/commands/command.py | 79 +++++++++++++++++++++---------- qutebrowser/commands/runners.py | 4 +- qutebrowser/config/config.py | 5 +- qutebrowser/utils/utilcmds.py | 4 +- scripts/src2asciidoc.py | 4 +- 9 files changed, 91 insertions(+), 56 deletions(-) diff --git a/doc/HACKING.asciidoc b/doc/HACKING.asciidoc index 550db683c..46f6cfd78 100644 --- a/doc/HACKING.asciidoc +++ b/doc/HACKING.asciidoc @@ -380,9 +380,6 @@ def foo(): The commands arguments are automatically deduced by inspecting your function. -If your function has a `count` argument with a default, the command will -support a count which will be passed in the argument. - If the function is a method of a class, the `@cmdutils.register` decorator needs to have an `instance=...` parameter which points to the (single/main) instance of the class. diff --git a/qutebrowser/browser/commands.py b/qutebrowser/browser/commands.py index f71b7e426..333195d6e 100644 --- a/qutebrowser/browser/commands.py +++ b/qutebrowser/browser/commands.py @@ -147,7 +147,8 @@ class CommandDispatcher: else: return None - def _scroll_percent(self, perc=None, count=None, orientation=None): + def _scroll_percent(self, perc=None, count: {'special': 'count'}=None, + orientation=None): """Inner logic for scroll_percent_(x|y). Args: @@ -246,7 +247,8 @@ class CommandDispatcher: return None @cmdutils.register(instance='command-dispatcher', scope='window') - def tab_close(self, left=False, right=False, opposite=False, count=None): + def tab_close(self, left=False, right=False, opposite=False, + count: {'special': 'count'}=None): """Close the current/[count]th tab. Args: @@ -273,7 +275,8 @@ class CommandDispatcher: @cmdutils.register(instance='command-dispatcher', name='open', split=False, scope='window') - def openurl(self, url, bg=False, tab=False, window=False, count=None): + def openurl(self, url, bg=False, tab=False, window=False, + count: {'special': 'count'}=None): """Open a URL in the current/[count]th tab. Args: @@ -304,7 +307,7 @@ class CommandDispatcher: @cmdutils.register(instance='command-dispatcher', name='reload', scope='window') - def reloadpage(self, count=None): + def reloadpage(self, count: {'special': 'count'}=None): """Reload the current/[count]th tab. Args: @@ -315,7 +318,7 @@ class CommandDispatcher: tab.reload() @cmdutils.register(instance='command-dispatcher', scope='window') - def stop(self, count=None): + def stop(self, count: {'special': 'count'}=None): """Stop loading in the current/[count]th tab. Args: @@ -327,7 +330,7 @@ class CommandDispatcher: @cmdutils.register(instance='command-dispatcher', name='print', scope='window') - def printpage(self, preview=False, count=None): + def printpage(self, preview=False, count: {'special': 'count'}=None): """Print the current/[count]th tab. Args: @@ -389,7 +392,8 @@ class CommandDispatcher: widget.back() @cmdutils.register(instance='command-dispatcher', scope='window') - def back(self, tab=False, bg=False, window=False, count=1): + def back(self, tab=False, bg=False, window=False, + count: {'special': 'count'}=1): """Go back in the history of the current tab. Args: @@ -401,7 +405,8 @@ class CommandDispatcher: self._back_forward(tab, bg, window, count, forward=False) @cmdutils.register(instance='command-dispatcher', scope='window') - def forward(self, tab=False, bg=False, window=False, count=1): + def forward(self, tab=False, bg=False, window=False, + count: {'special': 'count'}=1): """Go forward in the history of the current tab. Args: @@ -509,7 +514,7 @@ class CommandDispatcher: @cmdutils.register(instance='command-dispatcher', hide=True, scope='window') - def scroll(self, dx: float, dy: float, count=1): + def scroll(self, dx: float, dy: float, count: {'special': 'count'}=1): """Scroll the current tab by 'count * dx/dy'. Args: @@ -526,7 +531,8 @@ class CommandDispatcher: @cmdutils.register(instance='command-dispatcher', hide=True, scope='window') def scroll_perc(self, perc: float=None, - horizontal: {'flag': 'x'}=False, count=None): + horizontal: {'flag': 'x'}=False, + count: {'special': 'count'}=None): """Scroll to a specific percentage of the page. The percentage can be given either as argument or as count. @@ -542,7 +548,7 @@ class CommandDispatcher: @cmdutils.register(instance='command-dispatcher', hide=True, scope='window') - def scroll_page(self, x: float, y: float, count=1): + def scroll_page(self, x: float, y: float, count: {'special': 'count'}=1): """Scroll the frame page-wise. Args: @@ -584,7 +590,7 @@ class CommandDispatcher: message.info(self._win_id, "{} yanked to {}".format(what, target)) @cmdutils.register(instance='command-dispatcher', scope='window') - def zoom_in(self, count=1): + def zoom_in(self, count: {'special': 'count'}=1): """Increase the zoom level for the current tab. Args: @@ -594,7 +600,7 @@ class CommandDispatcher: tab.zoom(count) @cmdutils.register(instance='command-dispatcher', scope='window') - def zoom_out(self, count=1): + def zoom_out(self, count: {'special': 'count'}=1): """Decrease the zoom level for the current tab. Args: @@ -604,7 +610,7 @@ class CommandDispatcher: tab.zoom(-count) @cmdutils.register(instance='command-dispatcher', scope='window') - def zoom(self, zoom=None, count=None): + def zoom(self, zoom=None, count: {'special': 'count'}=None): """Set the zoom level for the current tab. The zoom can be given as argument or as [count]. If neither of both is @@ -650,7 +656,7 @@ class CommandDispatcher: raise cmdexc.CommandError("Nothing to undo!") @cmdutils.register(instance='command-dispatcher', scope='window') - def tab_prev(self, count=1): + def tab_prev(self, count: {'special': 'count'}=1): """Switch to the previous tab, or switch [count] tabs back. Args: @@ -665,7 +671,7 @@ class CommandDispatcher: raise cmdexc.CommandError("First tab") @cmdutils.register(instance='command-dispatcher', scope='window') - def tab_next(self, count=1): + def tab_next(self, count: {'special': 'count'}=1): """Switch to the next tab, or switch [count] tabs forward. Args: @@ -707,7 +713,8 @@ class CommandDispatcher: self._open(url, tab, bg, window) @cmdutils.register(instance='command-dispatcher', scope='window') - def tab_focus(self, index: (int, 'last')=None, count=None): + def tab_focus(self, index: (int, 'last')=None, + count: {'special': 'count'}=None): """Select the tab given as argument/[count]. Args: @@ -731,7 +738,8 @@ class CommandDispatcher: idx)) @cmdutils.register(instance='command-dispatcher', scope='window') - def tab_move(self, direction: ('+', '-')=None, count=None): + def tab_move(self, direction: ('+', '-')=None, + count: {'special': 'count'}=None): """Move the current tab. Args: diff --git a/qutebrowser/browser/downloads.py b/qutebrowser/browser/downloads.py index ca487913b..d47ea020a 100644 --- a/qutebrowser/browser/downloads.py +++ b/qutebrowser/browser/downloads.py @@ -359,7 +359,7 @@ class DownloadManager(QAbstractListModel): self.fetch(reply) @cmdutils.register(instance='download-manager') - def cancel_download(self, count=1): + def cancel_download(self, count: {'special': 'count'}=1): """Cancel the first/[count]th download. Args: diff --git a/qutebrowser/browser/quickmarks.py b/qutebrowser/browser/quickmarks.py index f818487ff..d446e5799 100644 --- a/qutebrowser/browser/quickmarks.py +++ b/qutebrowser/browser/quickmarks.py @@ -74,7 +74,7 @@ def prompt_save(win_id, url): @cmdutils.register() -def quickmark_add(win_id, url, name): +def quickmark_add(win_id: {'special': 'win_id'}, url, name): """Add a new quickmark. Args: diff --git a/qutebrowser/commands/command.py b/qutebrowser/commands/command.py index f25bfe349..96fa2aabb 100644 --- a/qutebrowser/commands/command.py +++ b/qutebrowser/commands/command.py @@ -42,12 +42,14 @@ class Command: completion: Completions to use for arguments, as a list of strings. debug: Whether this is a debugging command (only shown with --debug). parser: The ArgumentParser to use to parse this command. + special_params: A SpecialParams namedtuple with the names of the + special parameters, or None. _type_conv: A mapping of conversion functions for arguments. _name_conv: A mapping of argument names to parameter names. _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. - _count: Whether the command supports a count, or not. + _count: The count set for the command. _instance: The object to bind 'self' to. _scope: The scope to get _instance for in the object registry. @@ -57,8 +59,12 @@ class Command: """ AnnotationInfo = collections.namedtuple('AnnotationInfo', - ['kwargs', 'typ', 'name', 'flag']) + ['kwargs', 'typ', 'name', 'flag', + 'special']) ParamType = usertypes.enum('ParamType', ['flag', 'positional']) + SpecialParams = collections.namedtuple('SpecialParams', + ['count', 'win_id']) + def __init__(self, name, split, hide, instance, completion, modes, not_modes, needs_js, is_debug, ignore_args, @@ -89,8 +95,8 @@ class Command: self.namespace = None self._count = None self.pos_args = [] - has_count, desc, type_conv, name_conv = self._inspect_func() - self.has_count = has_count + special_params, desc, type_conv, name_conv = self._inspect_func() + self.special_params = special_params self.desc = desc self._type_conv = type_conv self._name_conv = name_conv @@ -164,20 +170,16 @@ class Command: """Inspect the function to get useful informations from it. Return: - A (has_count, desc, parser, type_conv) tuple. - has_count: Whether the command supports a count. + A (special_params, desc, parser, type_conv) tuple. + special_params: A SpecialParams namedtuple. desc: The description of the command. type_conv: A mapping of args to type converter callables. name_conv: A mapping of names to convert. """ type_conv = {} name_conv = {} + special_params = {'count': None, 'win_id': None} signature = inspect.signature(self.handler) - has_count = 'count' in signature.parameters - if has_count and (signature.parameters['count'].default is - inspect.Parameter.empty): - raise TypeError("{}: handler has count parameter without " - "default!".format(self.name)) doc = inspect.getdoc(self.handler) if doc is not None: desc = doc.splitlines()[0].strip() @@ -185,9 +187,34 @@ class Command: desc = "" if not self.ignore_args: for param in signature.parameters.values(): - if param.name in ('self', 'count', 'win_id'): - continue annotation_info = self._parse_annotation(param) + if param.name == 'self': + continue + special = annotation_info.special + if special == 'count': + if special_params['count'] is not None: + raise ValueError("Registered multiple parameters " + "({}/{}) as count!".format( + special_params['count'], + param.name)) + if param.default is inspect.Parameter.empty: + raise TypeError("{}: handler has count parameter " + "without default!".format(self.name)) + special_params['count'] = param.name + continue + elif special == 'win_id': + if special_params['win_id'] is not None: + raise ValueError("Registered multiple parameters " + "({}/{}) as win_id!".format( + special_params['win_id'], + param.name)) + special_params['win_id'] = param.name + continue + elif special is None: + pass + else: + raise ValueError("{}: Invalid value '{}' for 'special' " + "annotation!".format(self.name, special)) typ = self._get_type(param, annotation_info) args, kwargs = self._param_to_argparse_args( param, annotation_info) @@ -199,7 +226,8 @@ class Command: log.commands.vdebug('Adding arg {} of type {} -> {}'.format( param.name, typ, callsig)) self.parser.add_argument(*args, **kwargs) - return has_count, desc, type_conv, name_conv + special_params = self.SpecialParams(**special_params) + return special_params, desc, type_conv, name_conv def _param_to_argparse_args(self, param, annotation_info): """Get argparse arguments for a parameter. @@ -267,17 +295,18 @@ class Command: Return: An AnnotationInfo namedtuple. kwargs: A dict of keyword args to add to the - argparse.ArgumentParser.add_argument call. + argparse.ArgumentParser.add_argument call. typ: The type to use for this argument. flag: The short name/flag if overridden. name: The long name if overridden. """ - info = {'kwargs': {}, 'typ': None, 'flag': None, 'name': None} + info = {'kwargs': {}, 'typ': None, 'flag': None, 'name': None, + 'special': None} if param.annotation is not inspect.Parameter.empty: log.commands.vdebug("Parsing annotation {}".format( param.annotation)) if isinstance(param.annotation, dict): - for field in ('type', 'flag', 'name'): + for field in ('type', 'flag', 'name', 'special'): if field in param.annotation: info[field] = param.annotation[field] del param.annotation[field] @@ -330,10 +359,10 @@ class Command: args.append(param.default) elif param.kind == inspect.Parameter.KEYWORD_ONLY: if self._count is not None: - kwargs['count'] = self._count + kwargs[param.name] = self._count else: raise TypeError("{}: invalid parameter type {} for argument " - "'count'!".format(self.name, param.kind)) + "{!r}!".format(self.name, param.kind, param.name)) def _get_win_id_arg(self, win_id, param, args, kwargs): """Add the win_id argument to a function call. @@ -347,10 +376,10 @@ class Command: if param.kind == inspect.Parameter.POSITIONAL_OR_KEYWORD: args.append(win_id) elif param.kind == inspect.Parameter.KEYWORD_ONLY: - kwargs['win_id'] = win_id + kwargs[param.name] = win_id else: raise TypeError("{}: invalid parameter type {} for argument " - "'count'!".format(self.name, param.kind)) + "{!r}!".format(self.name, param.kind, param.name)) def _get_param_name_and_value(self, param): """Get the converted name and value for an inspect.Parameter.""" @@ -389,12 +418,12 @@ class Command: # Special case for 'self'. self._get_self_arg(win_id, param, args) continue - elif param.name == 'count': - # Special case for 'count'. + elif param.name == self.special_params.count: + # Special case for count parameter. self._get_count_arg(param, args, kwargs) continue - elif param.name == 'win_id': - # Special case for 'win_id'. + elif param.name == self.special_params.win_id: + # Special case for win_id parameter. self._get_win_id_arg(win_id, param, args, kwargs) continue name, value = self._get_param_name_and_value(param) diff --git a/qutebrowser/commands/runners.py b/qutebrowser/commands/runners.py index 001b5aa12..c28c0b96f 100644 --- a/qutebrowser/commands/runners.py +++ b/qutebrowser/commands/runners.py @@ -116,7 +116,7 @@ class SearchRunner(QObject): self._search(text, rev=True) @cmdutils.register(instance='search-runner', hide=True, scope='window') - def search_next(self, count=1): + def search_next(self, count: {'special': 'count'}=1): """Continue the search to the ([count]th) next term. Args: @@ -127,7 +127,7 @@ class SearchRunner(QObject): self.do_search.emit(self._text, self._flags) @cmdutils.register(instance='search-runner', hide=True, scope='window') - def search_prev(self, count=1): + def search_prev(self, count: {'special': 'count'}=1): """Continue the search to the ([count]th) previous term. Args: diff --git a/qutebrowser/config/config.py b/qutebrowser/config/config.py index ea7c6b162..ac921350a 100644 --- a/qutebrowser/config/config.py +++ b/qutebrowser/config/config.py @@ -442,8 +442,9 @@ class ConfigManager(QObject): @cmdutils.register(name='set', instance='config', completion=[Completion.section, Completion.option, Completion.value]) - def set_command(self, win_id, sectname: {'name': 'section'}, - optname: {'name': 'option'}, value=None, temp=False): + def set_command(self, win_id: {'special': 'win_id'}, + sectname: {'name': 'section'}, optname: {'name': 'option'}, + value=None, temp=False): """Set an option. If the option name ends with '?', the value of the option is shown diff --git a/qutebrowser/utils/utilcmds.py b/qutebrowser/utils/utilcmds.py index 5ba6c4bdf..05c167dd0 100644 --- a/qutebrowser/utils/utilcmds.py +++ b/qutebrowser/utils/utilcmds.py @@ -30,7 +30,7 @@ from qutebrowser.config import style @cmdutils.register(scope='window') -def later(ms: int, *command, win_id): +def later(ms: int, *command, win_id: {'special': 'win_id'}): """Execute a command after some time. Args: @@ -60,7 +60,7 @@ def later(ms: int, *command, win_id): @cmdutils.register(scope='window') -def repeat(times: int, *command, win_id): +def repeat(times: int, *command, win_id: {'special': 'win_id'}): """Repeat a given command. Args: diff --git a/scripts/src2asciidoc.py b/scripts/src2asciidoc.py index 67cb772e5..09df7d667 100755 --- a/scripts/src2asciidoc.py +++ b/scripts/src2asciidoc.py @@ -179,10 +179,10 @@ def _get_command_doc(name, cmd): raise KeyError("No description for arg {} of command " "'{}'!".format(e, cmd.name)) - if cmd.has_count: + if cmd.special_parameters.count is not None: output.append("") output.append("==== count") - output.append(parser.arg_descs['count']) + output.append(parser.arg_descs[cmd.special_params.count]) output.append("") output.append("")