2009-11-25 Eric Seidel <eric@webkit.org>
authoreric@webkit.org <eric@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 26 Nov 2009 02:22:27 +0000 (02:22 +0000)
committereric@webkit.org <eric@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 26 Nov 2009 02:22:27 +0000 (02:22 +0000)
        Reviewed by Adam Barth.

        'bugzilla-tool help' should only show common commands like how 'git help' does
        https://bugs.webkit.org/show_bug.cgi?id=31772

        I also took this opportunity to make 'help' a real Command.
        Making 'help' a real command required adding Command.tool (which we've wanted to do for a while).

        * Scripts/bugzilla-tool:
         - change should_show_command_help to should_show_in_main_help
        * Scripts/modules/commands/download.py:
         - Mark commands as being shown in main help or not.
         - show_in_main_help = False is not required (default is false),
           but it seemed to make the commands more self-documenting.
        * Scripts/modules/commands/queries.py: ditto
        * Scripts/modules/commands/queues.py: ditto
        * Scripts/modules/commands/upload.py: ditto
        * Scripts/modules/multicommandtool.py:
         - Make Command hold a pointer to tool in self.tool.  Most Command
           subclasses do not take advantage of this yet, but it was required
           for HelpCommand to be able to reach the tool from _help_epilog().
         - Move MultiCommandTool._standalone_help_for_command to Command.standalone_help
         - Move MultiCommandTool._help_epilog to Command._help_epilog
         - Move "help" logic into HelpCommand.execute()
         - Change should_show_command_help to should_show_in_main_help and add a default implementation.
        * Scripts/modules/multicommandtool_unittest.py:
         - Test hiding of Commands in --help, and that all commands are shown in 'help --all-commands'

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

WebKitTools/ChangeLog
WebKitTools/Scripts/bugzilla-tool
WebKitTools/Scripts/modules/commands/download.py
WebKitTools/Scripts/modules/commands/queries.py
WebKitTools/Scripts/modules/commands/queues.py
WebKitTools/Scripts/modules/commands/upload.py
WebKitTools/Scripts/modules/multicommandtool.py
WebKitTools/Scripts/modules/multicommandtool_unittest.py

index ab055747e3bcf564fd54e34b9b96e3dfe25b8462..5eb44272449cbb70b8819d8684a71d0d5e4dc9ef 100644 (file)
@@ -1,3 +1,33 @@
+2009-11-25  Eric Seidel  <eric@webkit.org>
+
+        Reviewed by Adam Barth.
+
+        'bugzilla-tool help' should only show common commands like how 'git help' does
+        https://bugs.webkit.org/show_bug.cgi?id=31772
+
+        I also took this opportunity to make 'help' a real Command.
+        Making 'help' a real command required adding Command.tool (which we've wanted to do for a while).
+
+        * Scripts/bugzilla-tool:
+         - change should_show_command_help to should_show_in_main_help
+        * Scripts/modules/commands/download.py:
+         - Mark commands as being shown in main help or not.
+         - show_in_main_help = False is not required (default is false),
+           but it seemed to make the commands more self-documenting.
+        * Scripts/modules/commands/queries.py: ditto
+        * Scripts/modules/commands/queues.py: ditto
+        * Scripts/modules/commands/upload.py: ditto
+        * Scripts/modules/multicommandtool.py:
+         - Make Command hold a pointer to tool in self.tool.  Most Command
+           subclasses do not take advantage of this yet, but it was required
+           for HelpCommand to be able to reach the tool from _help_epilog().
+         - Move MultiCommandTool._standalone_help_for_command to Command.standalone_help
+         - Move MultiCommandTool._help_epilog to Command._help_epilog
+         - Move "help" logic into HelpCommand.execute()
+         - Change should_show_command_help to should_show_in_main_help and add a default implementation.
+        * Scripts/modules/multicommandtool_unittest.py:
+         - Test hiding of Commands in --help, and that all commands are shown in 'help --all-commands'
+
 2009-11-25  Brian Weinstein  <bweinstein@apple.com>
 
         Reviewed by Dan Bernstein.
index fda3548bc426a326b94759cf8028442d51458585..8e0181d5f9972f728520dcca7152f48a27075121 100755 (executable)
@@ -71,7 +71,9 @@ class BugzillaTool(MultiCommandTool):
     def path(self):
         return __file__
 
-    def should_show_command_help(self, command):
+    def should_show_in_main_help(self, command):
+        if not command.show_in_main_help:
+            return False
         if command.requires_local_commits:
             return self.scm().supports_local_commits()
         return True
index 307bcdf1db82667598f611ec5593bd7d3ded45c1..d0eb26538ad3a83b1f3f418e6c037cbc0b413b81 100644 (file)
@@ -67,6 +67,7 @@ class BuildSequence(ConditionalLandingSequence):
 
 class Build(Command):
     name = "build"
+    show_in_main_help = False
     def __init__(self):
         options = WebKitLandingScripts.cleaning_options()
         options += WebKitLandingScripts.build_options()
@@ -80,6 +81,7 @@ class Build(Command):
 
 class ApplyAttachment(Command):
     name = "apply-attachment"
+    show_in_main_help = True
     def __init__(self):
         options = WebKitApplyingScripts.apply_options() + WebKitLandingScripts.cleaning_options()
         Command.__init__(self, "Apply an attachment to the local working directory", "ATTACHMENT_ID", options=options)
@@ -93,6 +95,7 @@ class ApplyAttachment(Command):
 
 class ApplyPatches(Command):
     name = "apply-patches"
+    show_in_main_help = True
     def __init__(self):
         options = WebKitApplyingScripts.apply_options() + WebKitLandingScripts.cleaning_options()
         Command.__init__(self, "Apply reviewed patches from provided bugs to the local working directory", "BUGID", options=options)
@@ -159,6 +162,7 @@ class LandDiffSequence(ConditionalLandingSequence):
 
 class LandDiff(Command):
     name = "land-diff"
+    show_in_main_help = True
     def __init__(self):
         options = [
             make_option("-r", "--reviewer", action="store", type="string", dest="reviewer", help="Update ChangeLogs to say Reviewed by REVIEWER."),
@@ -255,6 +259,7 @@ class CheckStyleSequence(LandingSequence):
 
 class CheckStyle(AbstractPatchProcessingCommand):
     name = "check-style"
+    show_in_main_help = False
     def __init__(self):
         options = WebKitLandingScripts.cleaning_options()
         options += WebKitLandingScripts.build_options()
@@ -284,6 +289,7 @@ class BuildAttachmentSequence(LandingSequence):
 
 class BuildAttachment(AbstractPatchProcessingCommand):
     name = "build-attachment"
+    show_in_main_help = False
     def __init__(self):
         options = WebKitLandingScripts.cleaning_options()
         options += WebKitLandingScripts.build_options()
@@ -319,6 +325,7 @@ class AbstractPatchLandingCommand(AbstractPatchProcessingCommand):
 
 class LandAttachment(AbstractPatchLandingCommand):
     name = "land-attachment"
+    show_in_main_help = True
     def __init__(self):
         AbstractPatchLandingCommand.__init__(self, "Land patches from bugzilla, optionally building and testing them first", "ATTACHMENT_ID [ATTACHMENT_IDS]")
 
@@ -328,6 +335,7 @@ class LandAttachment(AbstractPatchLandingCommand):
 
 class LandPatches(AbstractPatchLandingCommand):
     name = "land-patches"
+    show_in_main_help = True
     def __init__(self):
         AbstractPatchLandingCommand.__init__(self, "Land all patches on the given bugs, optionally building and testing them first", "BUGID [BUGIDS]")
 
@@ -342,6 +350,7 @@ class LandPatches(AbstractPatchLandingCommand):
 
 class Rollout(Command):
     name = "rollout"
+    show_in_main_help = True
     def __init__(self):
         options = WebKitLandingScripts.cleaning_options()
         options += WebKitLandingScripts.build_options()
index b30481154e023ba1af5714a9e9fa97b6bed72d54..2e66afb6940c6f5c4933b4352e00cebed9ff8c78 100644 (file)
@@ -56,6 +56,7 @@ from modules.workqueue import WorkQueue, WorkQueueDelegate
 
 class BugsToCommit(Command):
     name = "bugs-to-commit"
+    show_in_main_help = False
     def __init__(self):
         Command.__init__(self, "List bugs in the commit-queue")
 
@@ -67,6 +68,7 @@ class BugsToCommit(Command):
 
 class PatchesToCommit(Command):
     name = "patches-to-commit"
+    show_in_main_help = False
     def __init__(self):
         Command.__init__(self, "List patches in the commit-queue")
 
@@ -79,6 +81,7 @@ class PatchesToCommit(Command):
 
 class ReviewedPatches(Command):
     name = "reviewed-patches"
+    show_in_main_help = False
     def __init__(self):
         Command.__init__(self, "List r+'d patches on a bug", "BUGID")
 
@@ -91,6 +94,7 @@ class ReviewedPatches(Command):
 
 class TreeStatus(Command):
     name = "tree-status"
+    show_in_main_help = True
     def __init__(self):
         Command.__init__(self, "Print the status of the %s buildbots" % BuildBot.default_host)
 
index 1a5fa3361e67e5d3741e65364f220dbc01890946..27c4be8f11a62421f2bd679a94ad63a45a3ecd7f 100644 (file)
@@ -110,6 +110,7 @@ class AbstractQueue(Command, WorkQueueDelegate):
 
 class CommitQueue(AbstractQueue):
     name = "commit-queue"
+    show_in_main_help = False
     def __init__(self):
         AbstractQueue.__init__(self)
 
@@ -166,6 +167,7 @@ class AbstractTryQueue(AbstractQueue):
 
 class StyleQueue(AbstractTryQueue):
     name = "style-queue"
+    show_in_main_help = False
     def __init__(self):
         AbstractTryQueue.__init__(self)
 
@@ -178,6 +180,7 @@ class StyleQueue(AbstractTryQueue):
 
 class BuildQueue(AbstractTryQueue):
     name = "build-queue"
+    show_in_main_help = False
     def __init__(self):
         options = WebKitPort.port_options()
         AbstractTryQueue.__init__(self, options)
index 1561a5576380918b854390ddd4ef9bd48ed72ae4..cc52e3326b9664da9aba709332620c5359a66317 100644 (file)
@@ -56,6 +56,7 @@ from modules.workqueue import WorkQueue, WorkQueueDelegate
 
 class CommitMessageForCurrentDiff(Command):
     name = "commit-message"
+    show_in_main_help = False
     def __init__(self):
         Command.__init__(self, "Print a commit message suitable for the uncommitted changes")
 
@@ -66,6 +67,7 @@ class CommitMessageForCurrentDiff(Command):
 
 class ObsoleteAttachments(Command):
     name = "obsolete-attachments"
+    show_in_main_help = False
     def __init__(self):
         Command.__init__(self, "Mark all attachments on a bug as obsolete", "BUGID")
 
@@ -79,6 +81,7 @@ class ObsoleteAttachments(Command):
 
 class PostDiff(Command):
     name = "post-diff"
+    show_in_main_help = True
     def __init__(self):
         options = [
             make_option("-m", "--description", action="store", type="string", dest="description", help="Description string for the attachment (default: \"patch\")"),
@@ -120,6 +123,7 @@ class PostDiff(Command):
 
 class PostCommits(Command):
     name = "post-commits"
+    show_in_main_help = True
     def __init__(self):
         options = [
             make_option("-b", "--bug-id", action="store", type="string", dest="bug_id", help="Specify bug id if no URL is provided in the commit log."),
@@ -168,6 +172,7 @@ class PostCommits(Command):
 
 class CreateBug(Command):
     name = "create-bug"
+    show_in_main_help = True
     def __init__(self):
         options = [
             make_option("--cc", action="store", type="string", dest="cc", help="Comma-separated list of email addresses to carbon-copy."),
index 893cba26bbff6b21a4016e156fee6b2e516434c4..0475cf185dd0874f7ab7b2e830f8f55888dcbb7b 100644 (file)
@@ -40,6 +40,7 @@ from modules.logging import log
 
 class Command(object):
     name = None
+    # show_in_main_help = False # Subclasses must define show_in_main_help, we leave it out here to enforce that.
     def __init__(self, help_text, argument_names=None, options=None, requires_local_commits=False):
         self.help_text = help_text
         self.argument_names = argument_names
@@ -47,6 +48,14 @@ class Command(object):
         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
+        self.tool = None
+
+    # The tool calls bind_to_tool on each Command after adding it to its list.
+    def bind_to_tool(self, tool):
+        # Command instances can only be bound to one tool at a time.
+        if self.tool and tool != self.tool:
+            raise Exception("Command already bound to tool!")
+        self.tool = tool
 
     @staticmethod
     def _parse_required_arguments(argument_names):
@@ -89,6 +98,11 @@ class Command(object):
             return 1
         return self.execute(command_options, command_args, tool) or 0
 
+    def standalone_help(self):
+        help_text = self.name_with_arguments().ljust(len(self.name_with_arguments()) + 3) + self.help_text + "\n"
+        help_text += self.option_parser.format_option_help(IndentedHelpFormatter())
+        return help_text
+
     def execute(self, options, args, tool):
         raise NotImplementedError, "subclasses must implement"
 
@@ -101,7 +115,7 @@ class HelpPrintingOptionParser(OptionParser):
     def error(self, msg):
         self.print_usage(sys.stderr)
         error_message = "%s: error: %s\n" % (self.get_prog_name(), msg)
-        error_message += "\nType \"" + self.get_prog_name() + " --help\" to see usage.\n"
+        error_message += "\nType \"%s --help\" to see usage.\n" % self.get_prog_name()
         self.exit(1, error_message)
 
     # We override format_epilog to avoid the default formatting which would paragraph-wrap the epilog
@@ -112,11 +126,57 @@ class HelpPrintingOptionParser(OptionParser):
         return ""
 
 
+class HelpCommand(Command):
+    name = "help"
+    show_in_main_help = False
+
+    def __init__(self):
+        options = [
+            make_option("-a", "--all-commands", action="store_true", dest="show_all_commands", help="Print all available commands"),
+        ]
+        Command.__init__(self, "Display information about this program or its subcommands", "[COMMAND]", options=options)
+        self.show_all_commands = False # A hack used to pass --all-commands to _help_epilog even though it's called by the OptionParser.
+
+    def _help_epilog(self):
+        # Only show commands which are relevant to this checkout's SCM system.  Might this be confusing to some users?
+        if self.show_all_commands:
+            epilog = "All %prog commands:\n"
+            relevant_commands = self.tool.commands[:]
+        else:
+            epilog = "Common %prog commands:\n"
+            relevant_commands = filter(self.tool.should_show_in_main_help, self.tool.commands)
+        longest_name_length = max(map(lambda command: len(command.name), relevant_commands))
+        relevant_commands.sort(lambda a, b: cmp(a.name, b.name))
+        command_help_texts = map(lambda command: "   %s   %s\n" % (command.name.ljust(longest_name_length), command.help_text), relevant_commands)
+        epilog += "%s\n" % "".join(command_help_texts)
+        epilog += "See '%prog help --all-commands' to list all commands.\n"
+        epilog += "See '%prog help COMMAND' for more information on a specific command.\n"
+        return self.tool.global_option_parser.expand_prog_name(epilog)
+
+    def execute(self, options, args, tool):
+        if args:
+            command = self.tool.command_by_name(args[0])
+            if command:
+                print command.standalone_help()
+                return 0
+
+        self.show_all_commands = options.show_all_commands
+        tool.global_option_parser.print_help()
+        return 0
+
+
 class MultiCommandTool(object):
     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, prog=name, usage=self._usage_line())
+        self.help_command = self.command_by_name(HelpCommand.name)
+        # Require a help command, even if the manual test list doesn't include one.
+        if not self.help_command:
+            self.help_command = HelpCommand()
+            self.commands.append(self.help_command)
+        for command in self.commands:
+            command.bind_to_tool(self)
+        self.global_option_parser = HelpPrintingOptionParser(epilog_method=self.help_command._help_epilog, prog=name, usage=self._usage_line())
 
     @classmethod
     def _add_all_subclasses(cls, class_to_crawl, seen_classes):
@@ -135,26 +195,9 @@ class MultiCommandTool(object):
     def _usage_line():
         return "Usage: %prog [options] COMMAND [ARGS]"
 
-    @classmethod
-    def _standalone_help_for_command(cls, command):
-        help_text = command.name_with_arguments().ljust(len(command.name_with_arguments()) + 3) + command.help_text + "\n"
-        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)
-        longest_name_length = max(map(lambda command: len(command.name), relevant_commands))
-        relevant_commands.sort(lambda a, b: cmp(a.name, b.name))
-        command_help_texts = map(lambda command: "   %s   %s\n" % (command.name.ljust(longest_name_length), command.help_text), relevant_commands)
-        epilog = "%prog supports the following commands:\n"
-        epilog += "%s\n" % "".join(command_help_texts)
-        epilog += "See '%prog help COMMAND' for more information on a specific command.\n"
-        return self.global_option_parser.expand_prog_name(epilog)
-
     def handle_global_args(self, args):
         (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 "-".
@@ -186,8 +229,8 @@ class MultiCommandTool(object):
     def path(self):
         raise NotImplementedError, "subclasses must implement"
 
-    def should_show_command_help(self, command):
-        raise NotImplementedError, "subclasses must implement"
+    def should_show_in_main_help(self, command):
+        return command.show_in_main_help
 
     def should_execute_command(self, command):
         raise NotImplementedError, "subclasses must implement"
@@ -198,18 +241,7 @@ class MultiCommandTool(object):
         # Handle --help, etc:
         self.handle_global_args(global_args)
 
-        if not command_name:
-            self.global_option_parser.error("No command specified")
-
-        if command_name == "help":
-            if args_after_command_name:
-                command = self.command_by_name(args_after_command_name[0])
-                log(self._standalone_help_for_command(command))
-            else:
-                self.global_option_parser.print_help()
-            return 0
-
-        command = self.command_by_name(command_name)
+        command = self.command_by_name(command_name) or self.help_command
         if not command:
             self.global_option_parser.error("%s is not a recognized command" % command_name)
 
index 4a62f54ef5700d21e1c8317671ae5f710c779986..c71cc0982cfc2340b5170bf09cc3e380b25750e8 100644 (file)
@@ -35,12 +35,16 @@ from optparse import make_option
 
 class TrivialCommand(Command):
     name = "trivial"
+    show_in_main_help = True
     def __init__(self, **kwargs):
         Command.__init__(self, "help text", **kwargs)
 
     def execute(self, options, args, tool):
         pass
 
+class UncommonCommand(TrivialCommand):
+    name = "uncommon"
+    show_in_main_help = False
 
 class CommandTest(unittest.TestCase):
     def test_name_with_arguments(self):
@@ -76,11 +80,8 @@ class TrivialTool(MultiCommandTool):
     def path():
         return __file__
 
-    def should_show_command_help(self, command):
-        return True
-
     def should_execute_command(self, command):
-        return True
+        return (True, None)
 
 
 class MultiCommandToolTest(unittest.TestCase):
@@ -107,18 +108,50 @@ class MultiCommandToolTest(unittest.TestCase):
         self.assertEqual(tool.command_by_name("trivial").name, "trivial")
         self.assertEqual(tool.command_by_name("bar"), None)
 
-    def test_command_help(self):
-        command_with_options = TrivialCommand(options=[make_option("--my_option")])
-        tool = TrivialTool(commands=[command_with_options])
-
+    def _assert_tool_main_outputs(self, tool, main_args, expected_stdout, expected_stderr = "", exit_code=0):
         capture = OutputCapture()
         capture.capture_output()
-        exit_code = tool.main(["tool", "help", "trivial"])
+        exit_code = tool.main(main_args)
         (stdout_string, stderr_string) = capture.restore_output()
+        self.assertEqual(stdout_string, expected_stdout)
+        self.assertEqual(expected_stderr, expected_stderr)
+
+    def test_global_help(self):
+        tool = TrivialTool(commands=[TrivialCommand(), UncommonCommand()])
+        expected_common_commands_help = """Usage: trivial-tool [options] COMMAND [ARGS]
+
+Options:
+  -h, --help  show this help message and exit
+
+Common trivial-tool commands:
+   trivial   help text
+
+See 'trivial-tool help --all-commands' to list all commands.
+See 'trivial-tool help COMMAND' for more information on a specific command.
+
+"""
+        self._assert_tool_main_outputs(tool, ["tool", "help"], expected_common_commands_help)
+        expected_all_commands_help = """Usage: trivial-tool [options] COMMAND [ARGS]
+
+Options:
+  -h, --help  show this help message and exit
+
+All trivial-tool commands:
+   help       Display information about this program or its subcommands
+   trivial    help text
+   uncommon   help text
+
+See 'trivial-tool help --all-commands' to list all commands.
+See 'trivial-tool help COMMAND' for more information on a specific command.
+
+"""
+        self._assert_tool_main_outputs(tool, ["tool", "help", "--all-commands"], expected_all_commands_help)
+
+    def test_command_help(self):
+        command_with_options = TrivialCommand(options=[make_option("--my_option")])
+        tool = TrivialTool(commands=[command_with_options])
         expected_subcommand_help = "trivial [options]   help text\nOptions:\n  --my_option=MY_OPTION\n\n"
-        self.assertEqual(exit_code, 0)
-        self.assertEqual(stdout_string, "")
-        self.assertEqual(stderr_string, expected_subcommand_help)
+        self._assert_tool_main_outputs(tool, ["tool", "help", "trivial"], expected_subcommand_help)
 
 
 if __name__ == "__main__":