2009-11-25 Eric Seidel <eric@webkit.org>
authoreric@webkit.org <eric@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 25 Nov 2009 16:05:31 +0000 (16:05 +0000)
committereric@webkit.org <eric@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 25 Nov 2009 16:05:31 +0000 (16:05 +0000)
        Reviewed by Adam Barth.

        Centralize required argument parsing in Command
        https://bugs.webkit.org/show_bug.cgi?id=31872

        * Scripts/modules/commands/download.py: remove custom required arg message.
        * Scripts/modules/commands/upload.py: ditto.
        * Scripts/modules/multicommandtool.py:
         - Add _parse_required_arguments.
         - Pass program name off to OptionParser.
         - Add name() for access to tool name.
         - Add check_arguments_and_execute and make it return a return code.
         - Replace a couple uses of + with %.
        * Scripts/modules/multicommandtool_unittest.py: test _parse_required_arguments

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@51383 268f45cc-cd09-0410-ab3c-d52691b4dbfc

WebKitTools/ChangeLog
WebKitTools/Scripts/modules/commands/download.py
WebKitTools/Scripts/modules/commands/upload.py
WebKitTools/Scripts/modules/multicommandtool.py
WebKitTools/Scripts/modules/multicommandtool_unittest.py

index 31c4fbccc4ca217f680692a9da02fff7aa4d2e6f..e140f204b7a9073e86320157e21bd6abdc38df43 100644 (file)
@@ -1,3 +1,20 @@
+2009-11-25  Eric Seidel  <eric@webkit.org>
+
+        Reviewed by Adam Barth.
+
+        Centralize required argument parsing in Command
+        https://bugs.webkit.org/show_bug.cgi?id=31872
+
+        * Scripts/modules/commands/download.py: remove custom required arg message.
+        * Scripts/modules/commands/upload.py: ditto.
+        * Scripts/modules/multicommandtool.py:
+         - Add _parse_required_arguments.
+         - Pass program name off to OptionParser.
+         - Add name() for access to tool name.
+         - Add check_arguments_and_execute and make it return a return code.
+         - Replace a couple uses of + with %.
+        * Scripts/modules/multicommandtool_unittest.py: test _parse_required_arguments
+
 2009-11-25  Eric Seidel  <eric@webkit.org>
 
         Reviewed by Adam Barth.
index bd5edc6cb8769721bcc2a7ef092068db559fac2e..307bcdf1db82667598f611ec5593bd7d3ded45c1 100644 (file)
@@ -227,9 +227,6 @@ class AbstractPatchProcessingCommand(Command):
         return bugs_to_patches
 
     def execute(self, options, args, tool):
-        if not args:
-            error("%s required" % self.argument_names)
-
         self._prepare_to_process(options, args, tool)
         patches = self._fetch_list_of_patches_to_process(options, args, tool)
 
@@ -378,8 +375,6 @@ class Rollout(Command):
             log("No bugs were updated or re-opened to reflect this rollout.")
 
     def execute(self, options, args, tool):
-        if not args:
-            error("REVISION is required, see --help.")
         revision = args[0]
         bug_id = self._parse_bug_id_from_revision_diff(tool, revision)
         if options.complete_rollout:
index 7ec42385106919a65ff8f92f56c9923049ac57f0..1561a5576380918b854390ddd4ef9bd48ed72ae4 100644 (file)
@@ -142,9 +142,6 @@ class PostCommits(Command):
         return StringIO.StringIO(diff) # add_patch_to_bug expects a file-like object
 
     def execute(self, options, args, tool):
-        if not args:
-            error("%s argument is required" % self.argument_names)
-
         commit_ids = tool.scm().commit_ids_from_commitish_arguments(args)
         if len(commit_ids) > 10: # We could lower this limit, 10 is too many for one bug as-is.
             error("bugzilla-tool does not support attaching %s at once.  Are you sure you passed the right commit range?" % (pluralize("patch", len(commit_ids))))
index fbebb7341c7b34011346086f5a9e92ea4983680c..893cba26bbff6b21a4016e156fee6b2e516434c4 100644 (file)
@@ -35,6 +35,7 @@ import sys
 
 from optparse import OptionParser, IndentedHelpFormatter, SUPPRESS_USAGE, make_option
 
+from modules.grammar import pluralize
 from modules.logging import log
 
 class Command(object):
@@ -42,10 +43,27 @@ class Command(object):
     def __init__(self, help_text, argument_names=None, options=None, requires_local_commits=False):
         self.help_text = help_text
         self.argument_names = argument_names
+        self.required_arguments = self._parse_required_arguments(argument_names)
         self.options = options
         self.option_parser = HelpPrintingOptionParser(usage=SUPPRESS_USAGE, add_help_option=False, option_list=self.options)
         self.requires_local_commits = requires_local_commits
 
+    @staticmethod
+    def _parse_required_arguments(argument_names):
+        required_args = []
+        if not argument_names:
+            return required_args
+        split_args = argument_names.split(" ")
+        for argument in split_args:
+            if argument[0] == '[':
+                # For now our parser is rather dumb.  Do some minimal validation that
+                # we haven't confused it.
+                if argument[-1] != ']':
+                    raise Exception("Failure to parse argument string %s.  Argument %s is missing ending ]" % (argument_names, argument))
+            else:
+                required_args.append(argument)
+        return required_args
+
     def name_with_arguments(self):
         usage_string = self.name
         if self.options:
@@ -57,6 +75,20 @@ class Command(object):
     def parse_args(self, args):
         return self.option_parser.parse_args(args)
 
+    def check_arguments_and_execute(self, args_after_command_name, tool):
+        (command_options, command_args) = self.parse_args(args_after_command_name)
+
+        if len(command_args) < len(self.required_arguments):
+            log("%s required, %s provided.  Provided: %s  Required: %s\nSee '%s help %s' for usage." % (
+                pluralize("argument", len(self.required_arguments)),
+                pluralize("argument", len(command_args)),
+                "'%s'" % " ".join(command_args),
+                " ".join(self.required_arguments),
+                tool.name(),
+                self.name))
+            return 1
+        return self.execute(command_options, command_args, tool) or 0
+
     def execute(self, options, args, tool):
         raise NotImplementedError, "subclasses must implement"
 
@@ -81,10 +113,10 @@ class HelpPrintingOptionParser(OptionParser):
 
 
 class MultiCommandTool(object):
-    def __init__(self, commands=None):
+    def __init__(self, name=None, commands=None):
         # Allow the unit tests to disable command auto-discovery.
         self.commands = commands or [cls() for cls in self._find_all_commands() if cls.name]
-        self.global_option_parser = HelpPrintingOptionParser(epilog_method=self._help_epilog, usage=self._usage_line())
+        self.global_option_parser = HelpPrintingOptionParser(epilog_method=self._help_epilog, prog=name, usage=self._usage_line())
 
     @classmethod
     def _add_all_subclasses(cls, class_to_crawl, seen_classes):
@@ -109,6 +141,9 @@ class MultiCommandTool(object):
         help_text += command.option_parser.format_option_help(IndentedHelpFormatter())
         return help_text
 
+    def name(self):
+        return self.global_option_parser.get_prog_name()
+
     def _help_epilog(self):
         # Only show commands which are relevant to this checkout's SCM system.  Might this be confusing to some users?
         relevant_commands = filter(self.should_show_command_help, self.commands)
@@ -124,7 +159,7 @@ class MultiCommandTool(object):
         (options, args) = self.global_option_parser.parse_args(args)
         # We should never hit this because _split_args splits at the first arg without a leading "-".
         if args:
-            self.global_option_parser.error("Extra arguments before command: " + args)
+            self.global_option_parser.error("Extra arguments before command: %s" % args)
 
     @staticmethod
     def _split_args(args):
@@ -176,12 +211,11 @@ class MultiCommandTool(object):
 
         command = self.command_by_name(command_name)
         if not command:
-            self.global_option_parser.error(command_name + " is not a recognized command")
+            self.global_option_parser.error("%s is not a recognized command" % command_name)
 
         (should_execute, failure_reason) = self.should_execute_command(command)
         if not should_execute:
             log(failure_reason)
             return 0
 
-        (command_options, command_args) = command.parse_args(args_after_command_name)
-        return command.execute(command_options, command_args, self)
+        return command.check_arguments_and_execute(args_after_command_name, self)
index a7d59eca18b11efef557b1a721e31f3c88c9fa88..4a62f54ef5700d21e1c8317671ae5f710c779986 100644 (file)
@@ -50,10 +50,28 @@ class CommandTest(unittest.TestCase):
         command_with_args = TrivialCommand(options=[make_option("--my_option")])
         self.assertEqual(command_with_args.name_with_arguments(), "trivial [options]")
 
+    def test_parse_required_arguments(self):
+        self.assertEqual(Command._parse_required_arguments("ARG1 ARG2"), ["ARG1", "ARG2"])
+        self.assertEqual(Command._parse_required_arguments("[ARG1] [ARG2]"), [])
+        self.assertEqual(Command._parse_required_arguments("[ARG1] ARG2"), ["ARG2"])
+        # Note: We might make our arg parsing smarter in the future and allow this type of arguments string.
+        self.assertRaises(Exception, Command._parse_required_arguments, "[ARG1 ARG2]")
+
+    def test_required_arguments(self):
+        two_required_arguments = TrivialCommand(argument_names="ARG1 ARG2 [ARG3]")
+        capture = OutputCapture()
+        capture.capture_output()
+        exit_code = two_required_arguments.check_arguments_and_execute(["foo"], TrivialTool())
+        (stdout_string, stderr_string) = capture.restore_output()
+        expected_missing_args_error = "2 arguments required, 1 argument provided.  Provided: 'foo'  Required: ARG1 ARG2\nSee 'trivial-tool help trivial' for usage.\n"
+        self.assertEqual(exit_code, 1)
+        self.assertEqual(stdout_string, "")
+        self.assertEqual(stderr_string, expected_missing_args_error)
+
 
 class TrivialTool(MultiCommandTool):
     def __init__(self, commands=None):
-        MultiCommandTool.__init__(self, commands)
+        MultiCommandTool.__init__(self, name="trivial-tool", commands=commands)
 
     def path():
         return __file__