From c0594f37a19b67783556b2369906a0929c9dfec2 Mon Sep 17 00:00:00 2001 From: "iannucci@chromium.org" Date: Wed, 24 Jul 2013 23:43:47 +0000 Subject: [PATCH] Revert "Improve description layout. Improve coloring and add legend in help." This reverts commit 44a82fa84b4fa72070b79f44bed977307d364814. Broke git cl apply TBR=maruel@chromium.org BUG= Review URL: https://chromiumcodereview.appspot.com/20131006 git-svn-id: svn://svn.chromium.org/chrome/trunk/tools/depot_tools@213545 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, 147 insertions(+), 182 deletions(-) diff --git a/commit_queue.py b/commit_queue.py index 7639065f8..d23cbd1ac 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=None, values=None): - options, args = old_parse_args(args, values) + def new_parse_args(args): + options, args = old_parse_args(args) if not options.issue: parser.error('Require --issue') obj = rietveld.Rietveld(options.server, options.user, None) @@ -103,28 +103,32 @@ def CMDclear(parser, args): ## Boilerplate code -class OptionParser(optparse.OptionParser): - """An OptionParser instance with default options. +def gen_parser(): + """Returns an OptionParser instance with default options. It should be then processed with gen_usage() before being used. """ - 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] + 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) logging.basicConfig( - level=levels[min(len(levels) - 1, options.verbose)], + level=[logging.WARNING, logging.INFO, logging.DEBUG][ + min(2, options.verbose)], format='%(levelname)s %(filename)s(%(lineno)d): %(message)s') return options, args - def format_description(self, _): - """Removes description formatting.""" - return self.description.rstrip() + '\n' + parser.parse_args = Parse + + parser.add_option( + '-v', '--verbose', action='count', default=0, + help='Use multiple times to increase logging level') + return parser def Command(name): @@ -164,7 +168,7 @@ def gen_usage(parser, command): def main(args=None): # Do it late so all commands are listed. # pylint: disable=E1101 - parser = OptionParser(version=__version__) + parser = gen_parser() if args is None: args = sys.argv[1:] if args: diff --git a/gclient.py b/gclient.py index 50bed88ac..789d89b9d 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(OptionParser(), [ + return CMDrecurse(Parser(), [ '--jobs=%d' % options.jobs, '--scm=git', 'git', 'fetch'] + args) @@ -1735,57 +1735,52 @@ def GenUsage(parser, command): parser.epilog = getattr(obj, 'epilog', None) -class OptionParser(optparse.OptionParser): +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 gclientfile_default = os.environ.get('GCLIENT_FILE', '.gclient') - - 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)], + 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, format='%(module)s(%(lineno)d) %(funcName)s:%(message)s') if options.config_filename and options.spec: - self.error('Cannot specifiy both --gclientfile and --spec') + parser.error('Cannot specifiy both --gclientfile and --spec') if not options.config_filename: - options.config_filename = self.gclientfile_default + options.config_filename = gclientfile_default options.entries_filename = options.config_filename + '_entries' if options.jobs < 1: - self.error('--jobs must be 1 or higher') + parser.error('--jobs must be 1 or higher') # These hacks need to die. if not hasattr(options, 'revisions'): @@ -1804,10 +1799,10 @@ class OptionParser(optparse.OptionParser): if options.no_nag_max: gclient_scm.SCMWrapper.nag_max = None return (options, args) - - def format_epilog(self, _): - """Disables wordwrapping in epilog (usually examples).""" - return self.epilog or '' + parser.parse_args = Parse + # We don't want wordwrapping in epilog (usually examples) + parser.format_epilog = lambda _: parser.epilog or '' + return parser def Main(argv): @@ -1840,7 +1835,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 = OptionParser() + parser = Parser() if argv: command = Command(argv[0]) if command: diff --git a/git_cl.py b/git_cl.py index 1dcb1fe8c..60c41f4a8 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): - """Edits configuration for this tree.""" + """edit 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): - """Gets or sets base-url for this branch.""" + """get or set base-url for this branch""" branchref = RunGit(['symbolic-ref', 'HEAD']).strip() branch = ShortBranchName(branchref) _, args = parser.parse_args(args) @@ -1067,17 +1067,7 @@ def CMDbaseurl(parser, args): def CMDstatus(parser, args): - """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'. - """ + """show status of changelists""" parser.add_option('--field', help='print only specific field (desc|id|patch|url)') parser.add_option('-f', '--fast', action='store_true', @@ -1119,37 +1109,18 @@ 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() - 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)) + 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)) threads = [threading.Thread(target=fetch, args=(b,)) for b in branches] for t in threads: @@ -1159,16 +1130,25 @@ def CMDstatus(parser, args): # Do not use GetApprovingReviewers(), since it requires an HTTP request. for b in branches: c = Changelist(branchref=b) - url = c.GetIssueURL() - output.put((b, url, Fore.BLUE if url else Fore.WHITE)) + output.put((b, c.GetIssue(), None)) tmp = {} alignment = max(5, max(len(ShortBranchName(b)) for b in branches)) for branch in sorted(branches): while branch not in tmp: - b, i, color = output.get() - tmp[b] = (i, color) - issue, color = tmp.pop(branch) + 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 print ' %*s: %s%s%s' % ( alignment, ShortBranchName(branch), color, issue, Fore.RESET) @@ -1187,10 +1167,10 @@ def CMDstatus(parser, args): @usage('[issue_number]') def CMDissue(parser, args): - """Sets or displays the current code review issue number. + """Set or display the current code review issue number. Pass issue number 0 to clear the current issue. - """ +""" _, args = parser.parse_args(args) cl = Changelist() @@ -1206,7 +1186,7 @@ def CMDissue(parser, args): def CMDcomments(parser, args): - """Shows review comments of the current changelist.""" + """show review comments of the current changelist""" (_, args) = parser.parse_args(args) if args: parser.error('Unsupported argument: %s' % args) @@ -1232,7 +1212,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.') @@ -1257,7 +1237,7 @@ def CreateDescriptionFromLog(args): def CMDpresubmit(parser, args): - """Runs presubmit tests on the current changelist.""" + """run 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', @@ -1444,7 +1424,7 @@ def cleanup_list(l): @usage('[args to "git diff"]') def CMDupload(parser, args): - """Uploads the current changelist to codereview.""" + """upload 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', @@ -1767,7 +1747,7 @@ def SendUpstream(parser, args, cmd): @usage('[upstream branch to apply against]') def CMDdcommit(parser, args): - """Commits the current changelist via git-svn.""" + """commit 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 @@ -1783,7 +1763,7 @@ will instead be silently ignored.""" @usage('[upstream branch to apply against]') def CMDpush(parser, args): - """Commits the current changelist via git.""" + """commit 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\'?') @@ -1793,7 +1773,7 @@ def CMDpush(parser, args): @usage('') def CMDpatch(parser, args): - """Patchs in a code review.""" + """patch 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', @@ -1883,7 +1863,7 @@ def CMDpatch(parser, args): def CMDrebase(parser, args): - """Rebases current branch on top of svn repo.""" + """rebase 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 @@ -1921,7 +1901,7 @@ def GetTreeStatusReason(): def CMDtree(parser, args): - """Shows the status of the tree.""" + """show the status of the tree""" _, args = parser.parse_args(args) status = GetTreeStatus() if 'unset' == status: @@ -2037,7 +2017,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)) @@ -2055,7 +2035,7 @@ def CMDupstream(parser, args): def CMDset_commit(parser, args): - """Sets the commit bit to trigger the Commit Queue.""" + """set the commit bit""" _, args = parser.parse_args(args) if args: parser.error('Unrecognized args: %s' % ' '.join(args)) @@ -2065,7 +2045,7 @@ def CMDset_commit(parser, args): def CMDset_close(parser, args): - """Closes the issue.""" + """close the issue""" _, args = parser.parse_args(args) if args: parser.error('Unrecognized args: %s' % ' '.join(args)) @@ -2077,7 +2057,7 @@ def CMDset_close(parser, args): def CMDformat(parser, args): - """Runs clang-format on the diff.""" + """run 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) @@ -2174,7 +2154,7 @@ def Command(name): def CMDhelp(parser, args): - """Prints list of commands or help for a specific command.""" + """print list of commands or help for a specific command""" _, args = parser.parse_args(args) if len(args) == 1: return main(args + ['--help']) @@ -2192,32 +2172,11 @@ def GenUsage(parser, command): if command == 'help': command = '' else: - parser.description = obj.__doc__ + # OptParser.description prefer nicely non-formatted strings. + parser.description = re.sub('[\r\n ]{2,}', ' ', 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.""" @@ -2234,19 +2193,29 @@ 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, gen_summary(handler.__doc__).strip()) + (name, handler.__doc__.split('\n')[0].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)) - parser = OptionParser() + # 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 + if argv: command = Command(argv[0]) if command: diff --git a/tests/gclient_test.py b/tests/gclient_test.py index 73353dbc5..822a35c79 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.OptionParser() + parser = gclient.Parser() 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.OptionParser() + parser = gclient.Parser() 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.OptionParser() + parser = gclient.Parser() 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.OptionParser() + parser = gclient.Parser() 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.OptionParser() + parser = gclient.Parser() 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.OptionParser() + parser = gclient.Parser() 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.OptionParser() + parser = gclient.Parser() 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.OptionParser() + parser = gclient.Parser() 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.OptionParser() + parser = gclient.Parser() options, _ = parser.parse_args(['--jobs', '1']) options.deps_os = 'unix' @@ -552,7 +552,7 @@ class GclientTest(trial_dir.TestCase): ' "fuzz": "/fuzz",\n' '}') - options, _ = gclient.OptionParser().parse_args([]) + options, _ = gclient.Parser().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 ebcf14b13..288d58d05 100755 --- a/tests/trychange_unittest.py +++ b/tests/trychange_unittest.py @@ -47,8 +47,7 @@ class TryChangeUnittest(TryChangeTestsBase): def testMembersChanged(self): members = [ 'DieWithError', 'EPILOG', 'Escape', 'GIT', 'GetMungedDiff', 'GuessVCS', - 'HELP_STRING', 'InvalidScript', 'NoTryServerAccess', 'OptionParser', - 'PrintSuccess', + 'HELP_STRING', 'InvalidScript', 'NoTryServerAccess', '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 d0cdab69f..f67adef2c 100755 --- a/trychange.py +++ b/trychange.py @@ -524,15 +524,11 @@ 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 = OptionParser(usage=USAGE, version=__version__, prog=prog) + parser = optparse.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") @@ -679,6 +675,8 @@ 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)