From 1f1a2b6a38f1842de090601b5e7e1782ce3d7088 Mon Sep 17 00:00:00 2001 From: Benjy Weinberger Date: Fri, 7 Nov 2014 18:53:26 +0000 Subject: [PATCH] Remove a bunch of setup_parser plumbing from goal/task. reflect.py relied on the old options system via setup_parser, so it's been broken for a while, ever since options migration began. Fixed that by making it read from the new options. Testing Done: Manually ran the doc generator and inspected the resulting docs. CI passed. Reviewed at https://rbcommons.com/s/twitter/r/1297/ --- .../pants/backend/core/tasks/group_task.py | 18 ------ .../pants/backend/core/tasks/reflect.py | 60 ++++++++----------- src/python/pants/backend/core/tasks/task.py | 11 +--- src/python/pants/commands/goal_runner.py | 2 - src/python/pants/goal/goal.py | 36 ----------- src/python/pants/option/parser.py | 9 ++- tests/python/pants_test/tasks/test_base.py | 1 - 7 files changed, 35 insertions(+), 102 deletions(-) diff --git a/src/python/pants/backend/core/tasks/group_task.py b/src/python/pants/backend/core/tasks/group_task.py index c4c6bff68c8..4f43461107e 100644 --- a/src/python/pants/backend/core/tasks/group_task.py +++ b/src/python/pants/backend/core/tasks/group_task.py @@ -247,24 +247,6 @@ def register_options_on_scope(cls, options): for member_type in cls._member_types(): member_type.register_options_on_scope(options) - @classmethod - def setup_parser(cls, option_group, args, mkflag): - base_namespace = flag_namespace or mkflag.namespace - for member_type in cls._member_types(): - member_namespace = base_namespace + [member_type.name()] - mkflag = Mkflag(*member_namespace) - - # Hack to find the right option group. Ugly, but old options are - # going away soon anyway. - title = Goal.scope(cls.parent_options_scope, member_type.name()) - member_og = None - for og in option_group.parser.option_groups: - if og.title == title: - member_og = og - break - member_og = member_og or OptionGroup(option_group.parser, title=title) - member_type.setup_parser(member_og, args, mkflag) - @classmethod def product_types(cls): return product_type diff --git a/src/python/pants/backend/core/tasks/reflect.py b/src/python/pants/backend/core/tasks/reflect.py index d6d09fb48d3..b6a058ec565 100644 --- a/src/python/pants/backend/core/tasks/reflect.py +++ b/src/python/pants/backend/core/tasks/reflect.py @@ -5,12 +5,13 @@ from __future__ import (nested_scopes, generators, division, absolute_import, with_statement, print_function, unicode_literals) -from collections import defaultdict +import argparse import inspect import optparse import re from docutils.core import publish_parts +from pants.option.parser import Parser from twitter.common.collections.ordereddict import OrderedDict from pants.base.build_manual import get_builddict_info @@ -428,13 +429,16 @@ def gen_goals_glopts_reference_data(): return glopts -def gref_template_data_from_options(og): - """Get data for the Goals Reference from an optparse.OptionGroup""" - if not og: return None - title = og.title or "" - xref = "".join([c for c in title if c.isalnum()]) +def gref_template_data_from_options(scope, argparser): + """Get data for the Goals Reference from a CustomArgumentParser instance.""" + if not argparser: return None + title = scope or '' + xref = ''.join([c for c in title if c.isalnum()]) option_l = [] - for o in og.option_list: + for o in argparser.walk_actions(): + st = '/'.join(o.option_strings) + # Argparse elides the type in various circumstances, so we have to reverse that logic here. + typ = o.type or (type(o.const) if isinstance(o, argparse._StoreConstAction) else str) default = None if o.default and not str(o.default).startswith("('NO',"): default = o.default @@ -442,54 +446,42 @@ def gref_template_data_from_options(og): if o.help: hlp = indent_docstring_by_n(o.help.replace('[%default]', '').strip(), 6) option_l.append(TemplateData( - st=str(o), + st=st, default=default, hlp=hlp, - typ=o.type)) + typ=typ.__name__)) return TemplateData( title=title, options=option_l, xref=xref) - def gen_tasks_goals_reference_data(): """Generate the template data for the goals reference rst doc.""" goal_dict = {} goal_names = [] for goal in Goal.all(): - parser = optparse.OptionParser(add_help_option=False) - Goal.setup_parser(parser, [], [goal]) - options_by_title = defaultdict(lambda: None) - for group in parser.option_groups: - options_by_title[group.title] = group - found_option_groups = set() tasks = [] for task_name in goal.ordered_task_names(): task_type = goal.task_type_by_name(task_name) doc_rst = indent_docstring_by_n(task_type.__doc__ or '', 2) doc_html = rst_to_html(dedent_docstring(task_type.__doc__)) - options_title = Goal.scope(goal.name, task_name) - og = options_by_title[options_title] - if og: - found_option_groups.add(options_title) - impl = '{0}.{1}'.format(task_type.__module__, task_type.__name__) + option_parser = Parser(env={}, config={}, scope='', parent_parser=None) + task_type.register_options(option_parser.register) + argparser = option_parser._help_argparser + scope = Goal.scope(goal.name, task_name) + # task_type may actually be a synthetic subclass of the authored class from the source code. + # We want to display the authored class's name in the docs (but note that we must use the + # subclass for registering options above) + for authored_task_type in task_type.mro(): + if authored_task_type.__module__ != 'abc': + break + impl = '{0}.{1}'.format(authored_task_type.__module__, authored_task_type.__name__) tasks.append(TemplateData( impl=impl, doc_html=doc_html, doc_rst=doc_rst, - ogroup=gref_template_data_from_options(og))) - - leftover_option_groups = [] - for group in parser.option_groups: - if group.title in found_option_groups: continue - leftover_option_groups.append(gref_template_data_from_options(group)) - leftover_options = [] - for option in parser.option_list: - leftover_options.append(TemplateData(st=str(option))) - goal_dict[goal.name] = TemplateData(goal=goal, - tasks=tasks, - leftover_opts=leftover_options, - leftover_ogs=leftover_option_groups) + ogroup=gref_template_data_from_options(scope, argparser))) + goal_dict[goal.name] = TemplateData(goal=goal, tasks=tasks) goal_names.append(goal.name) goals = [goal_dict[name] for name in sorted(goal_names, key=lambda x: x.lower())] diff --git a/src/python/pants/backend/core/tasks/task.py b/src/python/pants/backend/core/tasks/task.py index 0e73f56e316..db41738f7db 100644 --- a/src/python/pants/backend/core/tasks/task.py +++ b/src/python/pants/backend/core/tasks/task.py @@ -33,7 +33,7 @@ class TaskBase(AbstractClass): Provides the base lifecycle methods that allow a task to interact with the command line, other tasks and the user. The lifecycle is linear and run via the following sequence: - 1. setup_parser - expose command line flags + 1. register_options - declare options configurable via cmd-line flag or config file. 2. __init__ - distill configuration into the information needed to execute 3. prepare - request any products needed from goal dependencies @@ -74,15 +74,6 @@ def register_options(cls, register): to register options. """ - @classmethod - def setup_parser(cls, option_group, args, mkflag): - """Set up the legacy cmd-line parser. - - Subclasses can add flags to the pants command line using the given option group. - Flag names should be created with mkflag([name]) to ensure flags are properly name-spaced - amongst other tasks. - """ - def __init__(self, context, workdir): """Subclass __init__ methods, if defined, *must* follow this idiom: diff --git a/src/python/pants/commands/goal_runner.py b/src/python/pants/commands/goal_runner.py index 3b06cf2c59d..24a2f174b7d 100644 --- a/src/python/pants/commands/goal_runner.py +++ b/src/python/pants/commands/goal_runner.py @@ -234,8 +234,6 @@ def setup_parser(self, parser, args): args[:] = augmented_args sys.stderr.write("(using pantsrc expansion: pants goal %s)\n" % ' '.join(augmented_args)) - Goal.setup_parser(parser, args, self.goals) - def run(self, lock): # TODO(John Sirois): Consider moving to straight python logging. The divide between the # context/work-unit logging and standard python logging doesn't buy us anything. diff --git a/src/python/pants/goal/goal.py b/src/python/pants/goal/goal.py index 44e0cc0bcac..470b0ba13bd 100644 --- a/src/python/pants/goal/goal.py +++ b/src/python/pants/goal/goal.py @@ -41,42 +41,6 @@ def scope(goal_name, task_name): """Returns options scope for specified task in specified goal.""" return goal_name if goal_name == task_name else '{0}.{1}'.format(goal_name, task_name) - @staticmethod - def setup_parser(parser, args, goals): - """Set up an OptionParser with options info for a goal and its deps. - This readies the parser to handle options for this goal and its deps. - It does not set up everything you might want for displaying help. - For that, you want setup_parser_for_help. - """ - visited = set() - - def do_setup_parser(goal): - if goal not in visited: - visited.add(goal) - for dep in goal.dependencies: - do_setup_parser(dep) - for task_name in goal.ordered_task_names(): - task_type = goal.task_type_by_name(task_name) - namespace = [task_name] if task_name == goal.name else [goal.name, task_name] - mkflag = Mkflag(*namespace) - title = task_type.options_scope - - # See if an option group already exists (created by the legacy options code - # in the new options system.) - option_group = None - for og in parser.option_groups: - if og.title == title: - option_group = og - break - - option_group = option_group or OptionGroup(parser, title=title) - task_type.setup_parser(option_group, args, mkflag) - if option_group.option_list: - parser.add_option_group(option_group) - - for goal in goals: - do_setup_parser(goal) - @staticmethod def all(): """Returns all registered goals, sorted alphabetically by name.""" diff --git a/src/python/pants/option/parser.py b/src/python/pants/option/parser.py index 410c1dde31b..46cf8004b2f 100644 --- a/src/python/pants/option/parser.py +++ b/src/python/pants/option/parser.py @@ -5,7 +5,7 @@ from __future__ import (nested_scopes, generators, division, absolute_import, with_statement, print_function, unicode_literals) -from argparse import ArgumentParser +from argparse import ArgumentParser, _HelpAction import copy from pants.option.arg_splitter import GLOBAL_SCOPE @@ -30,6 +30,13 @@ class CustomArgumentParser(ArgumentParser): def error(self, message): raise ParseError(message) + def walk_actions(self): + """Iterates over the argparse.Action objects for options registered on this parser.""" + for action_group in self._action_groups: + for action in action_group._group_actions: + if not isinstance(action, _HelpAction): + yield action + class Parser(object): """An argument parser in a hierarchy. diff --git a/tests/python/pants_test/tasks/test_base.py b/tests/python/pants_test/tasks/test_base.py index 8619214d793..41c32b5b35a 100644 --- a/tests/python/pants_test/tasks/test_base.py +++ b/tests/python/pants_test/tasks/test_base.py @@ -73,7 +73,6 @@ def prepare_task(self, task_type.options_scope = 'test' task_type.register_options_on_scope(new_options) - task_type.setup_parser(option_group, args, mkflag) old_options, _ = parser.parse_args(args or []) run_tracker = create_run_tracker()