From 44a82fa84b4fa72070b79f44bed977307d364814 Mon Sep 17 00:00:00 2001 From: "maruel@chromium.org" Date: Wed, 24 Jul 2013 13:56:49 +0000 Subject: [PATCH] Improve description layout. Improve coloring and add legend in help. Convert monkey patching to class inheritance for OptionParser usage in: commit_queue.py, gclient.py and trychange.py. Monkey patching confuses the hell out of pylint. R=jochen@chromium.org BUG= Review URL: https://chromiumcodereview.appspot.com/19552004 git-svn-id: svn://svn.chromium.org/chrome/trunk/tools/depot_tools@213423 0039d316-1c4b-4281-b951-d872f2087c98 --- commit_queue.py | 40 +++++---- gclient.py | 97 +++++++++++----------- git_cl.py | 157 +++++++++++++++++++++--------------- tests/gclient_test.py | 20 ++--- tests/trychange_unittest.py | 3 +- trychange.py | 12 +-- 6 files changed, 182 insertions(+), 147 deletions(-) diff --git a/commit_queue.py b/commit_queue.py index d23cbd1ac..7639065f8 100755 --- a/commit_queue.py +++ b/commit_queue.py @@ -34,8 +34,8 @@ def need_issue(fn): def hook(parser, args, *extra_args, **kwargs): old_parse_args = parser.parse_args - def new_parse_args(args): - options, args = old_parse_args(args) + def new_parse_args(args=None, values=None): + options, args = old_parse_args(args, values) if not options.issue: parser.error('Require --issue') obj = rietveld.Rietveld(options.server, options.user, None) @@ -103,32 +103,28 @@ def CMDclear(parser, args): ## Boilerplate code -def gen_parser(): - """Returns an OptionParser instance with default options. +class OptionParser(optparse.OptionParser): + """An OptionParser instance with default options. It should be then processed with gen_usage() before being used. """ - parser = optparse.OptionParser(version=__version__) - # Remove description formatting - parser.format_description = ( - lambda _: parser.description) # pylint: disable=E1101 - # Add common parsing. - old_parser_args = parser.parse_args - - def Parse(*args, **kwargs): - options, args = old_parser_args(*args, **kwargs) + def __init__(self, *args, **kwargs): + optparse.OptionParser.__init__(self, *args, **kwargs) + self.add_option( + '-v', '--verbose', action='count', default=0, + help='Use multiple times to increase logging level') + + def parse_args(self, args=None, values=None): + options, args = optparse.OptionParser.parse_args(self, args, values) + levels = [logging.WARNING, logging.INFO, logging.DEBUG] logging.basicConfig( - level=[logging.WARNING, logging.INFO, logging.DEBUG][ - min(2, options.verbose)], + level=levels[min(len(levels) - 1, options.verbose)], format='%(levelname)s %(filename)s(%(lineno)d): %(message)s') return options, args - parser.parse_args = Parse - - parser.add_option( - '-v', '--verbose', action='count', default=0, - help='Use multiple times to increase logging level') - return parser + def format_description(self, _): + """Removes description formatting.""" + return self.description.rstrip() + '\n' def Command(name): @@ -168,7 +164,7 @@ def gen_usage(parser, command): def main(args=None): # Do it late so all commands are listed. # pylint: disable=E1101 - parser = gen_parser() + parser = OptionParser(version=__version__) if args is None: args = sys.argv[1:] if args: diff --git a/gclient.py b/gclient.py index 789d89b9d..50bed88ac 100755 --- a/gclient.py +++ b/gclient.py @@ -1379,7 +1379,7 @@ def CMDfetch(parser, args): Completely git-specific. Simply runs 'git fetch [args ...]' for each module. """ (options, args) = parser.parse_args(args) - return CMDrecurse(Parser(), [ + return CMDrecurse(OptionParser(), [ '--jobs=%d' % options.jobs, '--scm=git', 'git', 'fetch'] + args) @@ -1735,52 +1735,57 @@ def GenUsage(parser, command): parser.epilog = getattr(obj, 'epilog', None) -def Parser(): - """Returns the default parser.""" - parser = optparse.OptionParser(version='%prog ' + __version__) - # some arm boards have issues with parallel sync. - if platform.machine().startswith('arm'): - jobs = 1 - else: - jobs = max(8, gclient_utils.NumLocalCpus()) - # cmp: 2013/06/19 - # Temporary workaround to lower bot-load on SVN server. - if os.environ.get('CHROME_HEADLESS') == '1': - jobs = 4 +class OptionParser(optparse.OptionParser): gclientfile_default = os.environ.get('GCLIENT_FILE', '.gclient') - parser.add_option('-j', '--jobs', default=jobs, type='int', - help='Specify how many SCM commands can run in parallel; ' - 'defaults to number of cpu cores (%default)') - parser.add_option('-v', '--verbose', action='count', default=0, - help='Produces additional output for diagnostics. Can be ' - 'used up to three times for more logging info.') - parser.add_option('--gclientfile', dest='config_filename', - default=None, - help='Specify an alternate %s file' % gclientfile_default) - parser.add_option('--spec', - default=None, - help='create a gclient file containing the provided ' - 'string. Due to Cygwin/Python brokenness, it ' - 'probably can\'t contain any newlines.') - parser.add_option('--no-nag-max', default=False, action='store_true', - help='If a subprocess runs for too long without generating' - ' terminal output, generate warnings, but do not kill' - ' the process.') - # Integrate standard options processing. - old_parser = parser.parse_args - def Parse(args): - (options, args) = old_parser(args) - level = [logging.ERROR, logging.WARNING, logging.INFO, logging.DEBUG][ - min(options.verbose, 3)] - logging.basicConfig(level=level, + + def __init__(self, **kwargs): + optparse.OptionParser.__init__( + self, version='%prog ' + __version__, **kwargs) + + # Some arm boards have issues with parallel sync. + if platform.machine().startswith('arm'): + jobs = 1 + else: + jobs = max(8, gclient_utils.NumLocalCpus()) + # cmp: 2013/06/19 + # Temporary workaround to lower bot-load on SVN server. + if os.environ.get('CHROME_HEADLESS') == '1': + jobs = 4 + + self.add_option( + '-j', '--jobs', default=jobs, type='int', + help='Specify how many SCM commands can run in parallel; defaults to ' + 'number of cpu cores (%default)') + self.add_option( + '-v', '--verbose', action='count', default=0, + help='Produces additional output for diagnostics. Can be used up to ' + 'three times for more logging info.') + self.add_option( + '--gclientfile', dest='config_filename', + help='Specify an alternate %s file' % self.gclientfile_default) + self.add_option( + '--spec', + help='create a gclient file containing the provided string. Due to ' + 'Cygwin/Python brokenness, it can\'t contain any newlines.') + self.add_option( + '--no-nag-max', default=False, action='store_true', + help='If a subprocess runs for too long without generating terminal ' + 'output, generate warnings, but do not kill the process.') + + def parse_args(self, args=None, values=None): + """Integrates standard options processing.""" + options, args = optparse.OptionParser.parse_args(self, args, values) + levels = [logging.ERROR, logging.WARNING, logging.INFO, logging.DEBUG] + logging.basicConfig( + level=levels[min(options.verbose, len(levels) - 1)], format='%(module)s(%(lineno)d) %(funcName)s:%(message)s') if options.config_filename and options.spec: - parser.error('Cannot specifiy both --gclientfile and --spec') + self.error('Cannot specifiy both --gclientfile and --spec') if not options.config_filename: - options.config_filename = gclientfile_default + options.config_filename = self.gclientfile_default options.entries_filename = options.config_filename + '_entries' if options.jobs < 1: - parser.error('--jobs must be 1 or higher') + self.error('--jobs must be 1 or higher') # These hacks need to die. if not hasattr(options, 'revisions'): @@ -1799,10 +1804,10 @@ def Parser(): if options.no_nag_max: gclient_scm.SCMWrapper.nag_max = None return (options, args) - parser.parse_args = Parse - # We don't want wordwrapping in epilog (usually examples) - parser.format_epilog = lambda _: parser.epilog or '' - return parser + + def format_epilog(self, _): + """Disables wordwrapping in epilog (usually examples).""" + return self.epilog or '' def Main(argv): @@ -1835,7 +1840,7 @@ def Main(argv): to_str(fn) for fn in dir(sys.modules[__name__]) if fn.startswith('CMD') ) CMDhelp.usage = '\n\nCommands are:\n' + '\n'.join(cmds) - parser = Parser() + parser = OptionParser() if argv: command = Command(argv[0]) if command: diff --git a/git_cl.py b/git_cl.py index 60c41f4a8..1dcb1fe8c 100755 --- a/git_cl.py +++ b/git_cl.py @@ -1033,7 +1033,7 @@ def DownloadHooks(force): @usage('[repo root containing codereview.settings]') def CMDconfig(parser, args): - """edit configuration for this tree""" + """Edits configuration for this tree.""" _, args = parser.parse_args(args) if len(args) == 0: @@ -1052,7 +1052,7 @@ def CMDconfig(parser, args): def CMDbaseurl(parser, args): - """get or set base-url for this branch""" + """Gets or sets base-url for this branch.""" branchref = RunGit(['symbolic-ref', 'HEAD']).strip() branch = ShortBranchName(branchref) _, args = parser.parse_args(args) @@ -1067,7 +1067,17 @@ def CMDbaseurl(parser, args): def CMDstatus(parser, args): - """show status of changelists""" + """Show status of changelists. + + Colors are used to tell the state of the CL unless --fast is used: + - Green LGTM'ed + - Blue waiting for review + - Yellow waiting for you to reply to review + - Red not sent for review or broken + - Cyan was committed, branch can be deleted + + Also see 'git cl comments'. + """ parser.add_option('--field', help='print only specific field (desc|id|patch|url)') parser.add_option('-f', '--fast', action='store_true', @@ -1109,18 +1119,37 @@ def CMDstatus(parser, args): if not options.fast: def fetch(b): + """Fetches information for an issue and returns (branch, issue, color).""" c = Changelist(branchref=b) i = c.GetIssueURL() - try: - props = c.GetIssueProperties() - r = c.GetApprovingReviewers() if i else None - if not props.get('messages'): - r = None - except urllib2.HTTPError: - # The issue probably doesn't exist anymore. - i += ' (broken)' - r = None - output.put((b, i, r)) + props = {} + r = None + if i: + try: + props = c.GetIssueProperties() + r = c.GetApprovingReviewers() if i else None + except urllib2.HTTPError: + # The issue probably doesn't exist anymore. + i += ' (broken)' + + msgs = props.get('messages') or [] + + if not i: + color = Fore.WHITE + elif props.get('closed'): + # Issue is closed. + color = Fore.CYAN + elif r: + # Was LGTM'ed. + color = Fore.GREEN + elif not msgs: + # No message was sent. + color = Fore.RED + elif msgs[-1]['sender'] != props.get('owner_email'): + color = Fore.YELLOW + else: + color = Fore.BLUE + output.put((b, i, color)) threads = [threading.Thread(target=fetch, args=(b,)) for b in branches] for t in threads: @@ -1130,25 +1159,16 @@ def CMDstatus(parser, args): # Do not use GetApprovingReviewers(), since it requires an HTTP request. for b in branches: c = Changelist(branchref=b) - output.put((b, c.GetIssue(), None)) + url = c.GetIssueURL() + output.put((b, url, Fore.BLUE if url else Fore.WHITE)) tmp = {} alignment = max(5, max(len(ShortBranchName(b)) for b in branches)) for branch in sorted(branches): while branch not in tmp: - b, i, r = output.get() - tmp[b] = (i, r) - issue, reviewers = tmp.pop(branch) - if not issue: - color = Fore.WHITE - elif reviewers: - # Was approved. - color = Fore.GREEN - elif reviewers is None: - # No message was sent. - color = Fore.RED - else: - color = Fore.BLUE + b, i, color = output.get() + tmp[b] = (i, color) + issue, color = tmp.pop(branch) print ' %*s: %s%s%s' % ( alignment, ShortBranchName(branch), color, issue, Fore.RESET) @@ -1167,10 +1187,10 @@ def CMDstatus(parser, args): @usage('[issue_number]') def CMDissue(parser, args): - """Set or display the current code review issue number. + """Sets or displays the current code review issue number. Pass issue number 0 to clear the current issue. -""" + """ _, args = parser.parse_args(args) cl = Changelist() @@ -1186,7 +1206,7 @@ def CMDissue(parser, args): def CMDcomments(parser, args): - """show review comments of the current changelist""" + """Shows review comments of the current changelist.""" (_, args) = parser.parse_args(args) if args: parser.error('Unsupported argument: %s' % args) @@ -1212,7 +1232,7 @@ def CMDcomments(parser, args): def CMDdescription(parser, args): - """brings up the editor for the current CL's description.""" + """Brings up the editor for the current CL's description.""" cl = Changelist() if not cl.GetIssue(): DieWithError('This branch has no associated changelist.') @@ -1237,7 +1257,7 @@ def CreateDescriptionFromLog(args): def CMDpresubmit(parser, args): - """run presubmit tests on the current changelist""" + """Runs presubmit tests on the current changelist.""" parser.add_option('-u', '--upload', action='store_true', help='Run upload hook instead of the push/dcommit hook') parser.add_option('-f', '--force', action='store_true', @@ -1424,7 +1444,7 @@ def cleanup_list(l): @usage('[args to "git diff"]') def CMDupload(parser, args): - """upload the current changelist to codereview""" + """Uploads the current changelist to codereview.""" parser.add_option('--bypass-hooks', action='store_true', dest='bypass_hooks', help='bypass upload presubmit hook') parser.add_option('--bypass-watchlists', action='store_true', @@ -1747,7 +1767,7 @@ def SendUpstream(parser, args, cmd): @usage('[upstream branch to apply against]') def CMDdcommit(parser, args): - """commit the current changelist via git-svn""" + """Commits the current changelist via git-svn.""" if not settings.GetIsGitSvn(): message = """This doesn't appear to be an SVN repository. If your project has a git mirror with an upstream SVN master, you probably need @@ -1763,7 +1783,7 @@ will instead be silently ignored.""" @usage('[upstream branch to apply against]') def CMDpush(parser, args): - """commit the current changelist via git""" + """Commits the current changelist via git.""" if settings.GetIsGitSvn(): print('This appears to be an SVN repository.') print('Are you sure you didn\'t mean \'git cl dcommit\'?') @@ -1773,7 +1793,7 @@ def CMDpush(parser, args): @usage('') def CMDpatch(parser, args): - """patch in a code review""" + """Patchs in a code review.""" parser.add_option('-b', dest='newbranch', help='create a new branch off trunk for the patch') parser.add_option('-f', action='store_true', dest='force', @@ -1863,7 +1883,7 @@ def CMDpatch(parser, args): def CMDrebase(parser, args): - """rebase current branch on top of svn repo""" + """Rebases current branch on top of svn repo.""" # Provide a wrapper for git svn rebase to help avoid accidental # git svn dcommit. # It's the only command that doesn't use parser at all since we just defer @@ -1901,7 +1921,7 @@ def GetTreeStatusReason(): def CMDtree(parser, args): - """show the status of the tree""" + """Shows the status of the tree.""" _, args = parser.parse_args(args) status = GetTreeStatus() if 'unset' == status: @@ -2017,7 +2037,7 @@ def CMDtry(parser, args): @usage('[new upstream branch]') def CMDupstream(parser, args): - """prints or sets the name of the upstream branch, if any""" + """Prints or sets the name of the upstream branch, if any.""" _, args = parser.parse_args(args) if len(args) > 1: parser.error('Unrecognized args: %s' % ' '.join(args)) @@ -2035,7 +2055,7 @@ def CMDupstream(parser, args): def CMDset_commit(parser, args): - """set the commit bit""" + """Sets the commit bit to trigger the Commit Queue.""" _, args = parser.parse_args(args) if args: parser.error('Unrecognized args: %s' % ' '.join(args)) @@ -2045,7 +2065,7 @@ def CMDset_commit(parser, args): def CMDset_close(parser, args): - """close the issue""" + """Closes the issue.""" _, args = parser.parse_args(args) if args: parser.error('Unrecognized args: %s' % ' '.join(args)) @@ -2057,7 +2077,7 @@ def CMDset_close(parser, args): def CMDformat(parser, args): - """run clang-format on the diff""" + """Runs clang-format on the diff.""" CLANG_EXTS = ['.cc', '.cpp', '.h'] parser.add_option('--full', action='store_true', default=False) opts, args = parser.parse_args(args) @@ -2154,7 +2174,7 @@ def Command(name): def CMDhelp(parser, args): - """print list of commands or help for a specific command""" + """Prints list of commands or help for a specific command.""" _, args = parser.parse_args(args) if len(args) == 1: return main(args + ['--help']) @@ -2172,11 +2192,32 @@ def GenUsage(parser, command): if command == 'help': command = '' else: - # OptParser.description prefer nicely non-formatted strings. - parser.description = re.sub('[\r\n ]{2,}', ' ', obj.__doc__) + parser.description = obj.__doc__ parser.set_usage('usage: %%prog %s [options] %s' % (command, more)) +class OptionParser(optparse.OptionParser): + """Creates the option parse and add --verbose support.""" + def __init__(self, *args, **kwargs): + optparse.OptionParser.__init__(self, *args, **kwargs) + self.add_option( + '-v', '--verbose', action='count', default=0, + help='Use 2 times for more debugging info') + + def parse_args(self, args=None, values=None): + options, args = optparse.OptionParser.parse_args(self, args, values) + levels = [logging.WARNING, logging.INFO, logging.DEBUG] + logging.basicConfig(level=levels[min(options.verbose, len(levels) - 1)]) + return options, args + + def format_description(self, _): + """Disables automatic reformatting.""" + lines = self.description.rstrip().splitlines() + lines_fixed = [lines[0]] + [l[2:] if len(l) >= 2 else l for l in lines[1:]] + description = ''.join(l + '\n' for l in lines_fixed) + return description[0].upper() + description[1:] + + def main(argv): """Doesn't parse the arguments here, just find the right subcommand to execute.""" @@ -2193,29 +2234,19 @@ def main(argv): # Do it late so all commands are listed. commands = Commands() length = max(len(c) for c in commands) + + def gen_summary(x): + """Creates a oneline summary from the docstring.""" + line = x.split('\n', 1)[0].rstrip('.') + return line[0].lower() + line[1:] + docs = sorted( - (name, handler.__doc__.split('\n')[0].strip()) + (name, gen_summary(handler.__doc__).strip()) for name, handler in commands.iteritems()) CMDhelp.usage_more = ('\n\nCommands are:\n' + '\n'.join( ' %-*s %s' % (length, name, doc) for name, doc in docs)) - # Create the option parse and add --verbose support. - parser = optparse.OptionParser() - parser.add_option( - '-v', '--verbose', action='count', default=0, - help='Use 2 times for more debugging info') - old_parser_args = parser.parse_args - def Parse(args): - options, args = old_parser_args(args) - if options.verbose >= 2: - logging.basicConfig(level=logging.DEBUG) - elif options.verbose: - logging.basicConfig(level=logging.INFO) - else: - logging.basicConfig(level=logging.WARNING) - return options, args - parser.parse_args = Parse - + parser = OptionParser() if argv: command = Command(argv[0]) if command: diff --git a/tests/gclient_test.py b/tests/gclient_test.py index 822a35c79..73353dbc5 100755 --- a/tests/gclient_test.py +++ b/tests/gclient_test.py @@ -86,7 +86,7 @@ class GclientTest(trial_dir.TestCase): Args: |jobs| is the number of parallel jobs simulated. """ - parser = gclient.Parser() + parser = gclient.OptionParser() options, args = parser.parse_args(['--jobs', jobs]) write( '.gclient', @@ -213,7 +213,7 @@ class GclientTest(trial_dir.TestCase): self.assertEquals('proto://host/path@revision', d.url) def testStr(self): - parser = gclient.Parser() + parser = gclient.OptionParser() options, _ = parser.parse_args([]) obj = gclient.GClient('foo', options) obj.add_dependencies_and_close( @@ -265,7 +265,7 @@ class GclientTest(trial_dir.TestCase): os.chdir(topdir) - parser = gclient.Parser() + parser = gclient.OptionParser() options, _ = parser.parse_args([]) options.force = True client = gclient.GClient.LoadCurrentConfig(options) @@ -314,7 +314,7 @@ class GclientTest(trial_dir.TestCase): os.chdir(topdir) - parser = gclient.Parser() + parser = gclient.OptionParser() options, _ = parser.parse_args([]) options.force = True client = gclient.GClient.LoadCurrentConfig(options) @@ -350,7 +350,7 @@ class GclientTest(trial_dir.TestCase): ' "baz": { "foo/dir3": "/dir3", },\n' '}') - parser = gclient.Parser() + parser = gclient.OptionParser() options, _ = parser.parse_args(['--jobs', '1']) options.deps_os = "unix" @@ -385,7 +385,7 @@ class GclientTest(trial_dir.TestCase): ' "baz": { "foo/dir3": "/dir3", },\n' '}') - parser = gclient.Parser() + parser = gclient.OptionParser() options, _ = parser.parse_args(['--jobs', '1']) options.deps_os = "unix" @@ -413,7 +413,7 @@ class GclientTest(trial_dir.TestCase): ' "unix": { "foo/dir2": "/dir2", },\n' '}') - parser = gclient.Parser() + parser = gclient.OptionParser() options, _ = parser.parse_args(['--jobs', '1']) options.deps_os = "unix" @@ -458,7 +458,7 @@ class GclientTest(trial_dir.TestCase): ' "jaz": { "bar/jaz": "/jaz", },\n' '}') - parser = gclient.Parser() + parser = gclient.OptionParser() options, _ = parser.parse_args(['--jobs', '1']) options.deps_os = 'unix' @@ -499,7 +499,7 @@ class GclientTest(trial_dir.TestCase): ' "jaz": { "foo/jaz": "/jaz", },\n' '}') - parser = gclient.Parser() + parser = gclient.OptionParser() options, _ = parser.parse_args(['--jobs', '1']) options.deps_os = 'unix' @@ -552,7 +552,7 @@ class GclientTest(trial_dir.TestCase): ' "fuzz": "/fuzz",\n' '}') - options, _ = gclient.Parser().parse_args([]) + options, _ = gclient.OptionParser().parse_args([]) obj = gclient.GClient.LoadCurrentConfig(options) obj.RunOnDeps('None', []) self.assertEquals( diff --git a/tests/trychange_unittest.py b/tests/trychange_unittest.py index 288d58d05..ebcf14b13 100755 --- a/tests/trychange_unittest.py +++ b/tests/trychange_unittest.py @@ -47,7 +47,8 @@ class TryChangeUnittest(TryChangeTestsBase): def testMembersChanged(self): members = [ 'DieWithError', 'EPILOG', 'Escape', 'GIT', 'GetMungedDiff', 'GuessVCS', - 'HELP_STRING', 'InvalidScript', 'NoTryServerAccess', 'PrintSuccess', + 'HELP_STRING', 'InvalidScript', 'NoTryServerAccess', 'OptionParser', + 'PrintSuccess', 'RunCommand', 'RunGit', 'SCM', 'SVN', 'TryChange', 'USAGE', 'breakpad', 'datetime', 'errno', 'fix_encoding', 'gcl', 'gclient_utils', 'gen_parser', 'getpass', 'json', 'logging', 'optparse', 'os', 'posixpath', 're', 'scm', diff --git a/trychange.py b/trychange.py index f67adef2c..d0cdab69f 100755 --- a/trychange.py +++ b/trychange.py @@ -524,11 +524,15 @@ def GetMungedDiff(path_diff, diff): return (diff, changed_files) +class OptionParser(optparse.OptionParser): + def format_epilog(self, _): + """Removes epilog formatting.""" + return self.epilog or '' + + def gen_parser(prog): # Parse argv - parser = optparse.OptionParser(usage=USAGE, - version=__version__, - prog=prog) + parser = OptionParser(usage=USAGE, version=__version__, prog=prog) parser.add_option("-v", "--verbose", action="count", default=0, help="Prints debugging infos") group = optparse.OptionGroup(parser, "Result and status") @@ -675,8 +679,6 @@ def TryChange(argv, if extra_epilog: epilog += extra_epilog parser.epilog = epilog - # Remove epilog formatting - parser.format_epilog = lambda x: parser.epilog options, args = parser.parse_args(argv)