From 42c5bbbc96b863cecc319ae78cb888555ee0128e Mon Sep 17 00:00:00 2001 From: Josip Sokcevic Date: Mon, 24 Jan 2022 21:42:28 +0000 Subject: [PATCH] Revert "Use pylint 2.7 for depot_tools" This reverts commit 22bf605bb63f8ff01aa7c1a2841718e3489b43d6. Reason for revert: breaks gclient sync Original change's description: > Use pylint 2.7 for depot_tools > > This includes a few fixes for specific errors, and disables several new > warnings introduced in this version, in order to allow for an incremental migration. > > Bug:1262286 > Change-Id: Ie97d686748c9c952e87718a65f401c5f6f80a5c9 > Reviewed-on: https://chromium-review.googlesource.com/c/chromium/tools/depot_tools/+/3400616 > Reviewed-by: Gavin Mak > Commit-Queue: Aravind Vasudevan Bug: 1262286 Change-Id: Ieb946073c7886c7bf056ce843a5a48e82becf7a5 No-Presubmit: true No-Tree-Checks: true No-Try: true Reviewed-on: https://chromium-review.googlesource.com/c/chromium/tools/depot_tools/+/3413672 Bot-Commit: Rubber Stamper Commit-Queue: Josip Sokcevic --- PRESUBMIT.py | 18 +---- autoninja.py | 4 +- cit.py | 4 +- clang_format_merge_driver.py | 2 - compile_single_file.py | 2 +- cpplint.py | 24 +++---- download_from_google_storage.py | 7 +- fetch.py | 25 +++---- gclient.py | 23 +++--- gclient_eval.py | 71 ++++++++----------- gclient_scm.py | 37 +++++----- gclient_utils.py | 40 +++++------ gerrit_util.py | 12 ++-- git_cache.py | 5 +- git_cl.py | 66 ++++++++--------- git_common.py | 3 +- git_footers.py | 7 +- git_map.py | 3 +- git_nav_downstream.py | 3 +- git_number.py | 4 +- gn.py | 3 +- metrics_utils.py | 6 +- my_activity.py | 50 +++++++------ owners_finder.py | 32 +++------ presubmit_canned_checks.py | 57 +++++++-------- presubmit_canned_checks_test_mocks.py | 2 +- presubmit_support.py | 11 ++- scm.py | 27 ++++--- subcommand.py | 4 +- testing_support/fake_repos.py | 2 +- tests/bot_update_coverage_test.py | 2 + .../download_from_google_storage_unittest.py | 8 +-- tests/gclient_scm_test.py | 8 +-- tests/gclient_smoketest_base.py | 2 + tests/git_find_releases_test.py | 6 +- tests/git_migrate_default_branch_test.py | 9 +-- tests/presubmit_unittest.py | 22 +++--- tests/subprocess2_test.py | 1 - upload_to_google_storage.py | 8 +-- 39 files changed, 277 insertions(+), 343 deletions(-) diff --git a/PRESUBMIT.py b/PRESUBMIT.py index 6571a4ee7..ef1c98781 100644 --- a/PRESUBMIT.py +++ b/PRESUBMIT.py @@ -57,27 +57,15 @@ def CheckPylint(input_api, output_api): files_to_skip.extend([fnmatch.translate(l) for l in lines if l and not l.startswith('#')]) disabled_warnings = [ - 'R0401', # Cyclic import - 'W0613', # Unused argument - 'C0415', # import-outside-toplevel - 'R1710', # inconsistent-return-statements - 'E1101', # no-member - 'E1120', # no-value-for-parameter - 'R1708', # stop-iteration-return - 'W1510', # subprocess-run-check - # Checks which should be re-enabled after Python 2 support is removed. - 'R0205', # useless-object-inheritance - 'R1725', # super-with-arguments - 'W0707', # raise-missing-from - 'W1113', # keyword-arg-before-vararg + 'R0401', # Cyclic import + 'W0613', # Unused argument ] return input_api.RunTests(input_api.canned_checks.GetPylint( input_api, output_api, files_to_check=files_to_check, files_to_skip=files_to_skip, - disabled_warnings=disabled_warnings, - version='2.7'), parallel=False) + disabled_warnings=disabled_warnings)) def CheckRecipes(input_api, output_api): diff --git a/autoninja.py b/autoninja.py index 28f9ff39a..4be1083ce 100755 --- a/autoninja.py +++ b/autoninja.py @@ -51,7 +51,7 @@ for index, arg in enumerate(input_args[1:]): elif arg.startswith('-C'): # Support -Cout/Default output_dir = arg[2:] - elif arg in ('-o', '--offline'): + elif arg == '-o' or arg == '--offline': offline = True elif arg == '-h': print('autoninja: Use -o/--offline to temporary disable goma.', @@ -59,7 +59,7 @@ for index, arg in enumerate(input_args[1:]): print(file=sys.stderr) # Strip -o/--offline so ninja doesn't see them. -input_args = [ arg for arg in input_args if arg not in ('-o', '--offline')] +input_args = [ arg for arg in input_args if arg != '-o' and arg != '--offline'] use_goma = False use_remoteexec = False diff --git a/cit.py b/cit.py index c4b475eb4..383b3230a 100755 --- a/cit.py +++ b/cit.py @@ -83,8 +83,8 @@ def is_exe(filename): """Given a full filepath, return true if the file is an executable.""" if sys.platform.startswith('win'): return filename.endswith('.exe') - - return os.path.isfile(filename) and os.access(filename, os.X_OK) + else: + return os.path.isfile(filename) and os.access(filename, os.X_OK) def get_available_tools(): diff --git a/clang_format_merge_driver.py b/clang_format_merge_driver.py index 764d398d6..2e5211c41 100755 --- a/clang_format_merge_driver.py +++ b/clang_format_merge_driver.py @@ -28,9 +28,7 @@ def main(): sys.argv[0]) sys.exit(1) - # pylint: disable=unbalanced-tuple-unpacking base, current, others, file_name_in_tree = sys.argv[1:5] - # pylint: enable=unbalanced-tuple-unpacking if file_name_in_tree == '%P': print(file=sys.stderr) diff --git a/compile_single_file.py b/compile_single_file.py index eba8e900c..bb0a5416d 100644 --- a/compile_single_file.py +++ b/compile_single_file.py @@ -23,7 +23,7 @@ def path_to_source_root(path): # to break when we rename directories. fingerprints = ['chrome', 'net', 'v8', 'build', 'skia'] while candidate and not all( - os.path.isdir(os.path.join(candidate, fp)) for fp in fingerprints): + [os.path.isdir(os.path.join(candidate, fp)) for fp in fingerprints]): new_candidate = os.path.dirname(candidate) if new_candidate == candidate: raise Exception("Couldn't find source-dir from %s" % path) diff --git a/cpplint.py b/cpplint.py index ca711206e..2e95327a2 100755 --- a/cpplint.py +++ b/cpplint.py @@ -5263,8 +5263,8 @@ _RE_PATTERN_STRING = re.compile(r'\bstring\b') _re_pattern_headers_maybe_templates = [] for _header, _templates in _HEADERS_MAYBE_TEMPLATES: for _template in _templates: - # Match max(..., ...), max(..., ...), but not foo->max, foo.max - # or type::max(). + # Match max(..., ...), max(..., ...), but not foo->max, foo.max or + # type::max(). _re_pattern_headers_maybe_templates.append( (re.compile(r'[^>.]\b' + _template + r'(<.*?>)?\([^\)]'), _template, @@ -5380,9 +5380,9 @@ def CheckForIncludeWhatYouUse(filename, clean_lines, include_state, error, io: The IO factory to use to read the header file. Provided for unittest injection. """ - # A map of header name to linenumber and the template entity. - # Example of required: { '': (1219, 'less<>') } - required = {} + required = {} # A map of header name to linenumber and the template entity. + # Example of required: { '': (1219, 'less<>') } + for linenum in range(clean_lines.NumLines()): line = clean_lines.elided[linenum] if not line or line[0] == '#': @@ -5865,9 +5865,9 @@ def ProcessConfigOverrides(filename): elif name == 'linelength': global _line_length try: - _line_length = int(val) + _line_length = int(val) except ValueError: - sys.stderr.write('Line length must be numeric.') + sys.stderr.write('Line length must be numeric.') else: sys.stderr.write( 'Invalid configuration option (%s) in file %s\n' % @@ -5881,7 +5881,7 @@ def ProcessConfigOverrides(filename): # Apply all the accumulated filters in reverse order (top-level directory # config options having the least priority). for filter in reversed(cfg_filters): - _AddFilters(filter) + _AddFilters(filter) return True @@ -6053,15 +6053,15 @@ def ParseArguments(args): elif opt == '--linelength': global _line_length try: - _line_length = int(val) + _line_length = int(val) except ValueError: - PrintUsage('Line length must be digits.') + PrintUsage('Line length must be digits.') elif opt == '--extensions': global _valid_extensions try: - _valid_extensions = set(val.split(',')) + _valid_extensions = set(val.split(',')) except ValueError: - PrintUsage('Extensions must be comma separated list.') + PrintUsage('Extensions must be comma separated list.') if not filenames: PrintUsage('No files were specified.') diff --git a/download_from_google_storage.py b/download_from_google_storage.py index 52ef97cd1..27a67c8de 100755 --- a/download_from_google_storage.py +++ b/download_from_google_storage.py @@ -44,10 +44,9 @@ PLATFORM_MAPPING = { 'aix7': 'aix', } -if sys.version_info.major == 2: - # pylint: disable=redefined-builtin - class FileNotFoundError(IOError): - pass + +class FileNotFoundError(IOError): + pass class InvalidFileError(IOError): diff --git a/fetch.py b/fetch.py index 74b229852..c4b605c66 100755 --- a/fetch.py +++ b/fetch.py @@ -56,6 +56,7 @@ class Checkout(object): def exists(self): """Check does this checkout already exist on desired location""" + pass def init(self): pass @@ -66,18 +67,18 @@ class Checkout(object): return '' if return_stdout: return subprocess.check_output(cmd, **kwargs).decode() - - try: - subprocess.check_call(cmd, **kwargs) - except subprocess.CalledProcessError as e: - # If the subprocess failed, it likely emitted its own distress message - # already - don't scroll that message off the screen with a stack trace - # from this program as well. Emit a terse message and bail out here; - # otherwise a later step will try doing more work and may hide the - # subprocess message. - print('Subprocess failed with return code %d.' % e.returncode) - sys.exit(e.returncode) - return '' + else: + try: + subprocess.check_call(cmd, **kwargs) + except subprocess.CalledProcessError as e: + # If the subprocess failed, it likely emitted its own distress message + # already - don't scroll that message off the screen with a stack trace + # from this program as well. Emit a terse message and bail out here; + # otherwise a later step will try doing more work and may hide the + # subprocess message. + print('Subprocess failed with return code %d.' % e.returncode) + sys.exit(e.returncode) + return '' class GclientCheckout(Checkout): diff --git a/gclient.py b/gclient.py index a7cddc780..446be28d1 100755 --- a/gclient.py +++ b/gclient.py @@ -238,7 +238,7 @@ class Hook(object): not gclient_eval.EvaluateCondition(self._condition, self._variables)): return - cmd = list(self._action) + cmd = [arg for arg in self._action] if cmd[0] == 'python': cmd[0] = 'vpython' @@ -373,8 +373,8 @@ class DependencySettings(object): def target_os(self): if self.local_target_os is not None: return tuple(set(self.local_target_os).union(self.parent.target_os)) - - return self.parent.target_os + else: + return self.parent.target_os @property def target_cpu(self): @@ -1420,7 +1420,7 @@ solutions = %(solution_list)s if 'all' in enforced_os: enforced_os = self.DEPS_OS_CHOICES.values() self._enforced_os = tuple(set(enforced_os)) - self._enforced_cpu = (detect_host_arch.HostArch(), ) + self._enforced_cpu = detect_host_arch.HostArch(), self._root_dir = root_dir self._cipd_root = None self.config_content = None @@ -1830,7 +1830,7 @@ it or fix the checkout. if command == 'update': gn_args_dep = self.dependencies[0] if gn_args_dep._gn_args_from: - deps_map = {dep.name: dep for dep in gn_args_dep.dependencies} + deps_map = dict([(dep.name, dep) for dep in gn_args_dep.dependencies]) gn_args_dep = deps_map.get(gn_args_dep._gn_args_from) if gn_args_dep and gn_args_dep.HasGNArgsFile(): gn_args_dep.WriteGNArgsFile() @@ -1874,12 +1874,11 @@ it or fix the checkout. entries = {} def GrabDeps(dep): """Recursively grab dependencies.""" - for rec_d in dep.dependencies: - rec_d.PinToActualRevision() - if ShouldPrintRevision(rec_d): - entries[rec_d.name] = rec_d.url - GrabDeps(rec_d) - + for d in dep.dependencies: + d.PinToActualRevision() + if ShouldPrintRevision(d): + entries[d.name] = d.url + GrabDeps(d) GrabDeps(d) json_output.append({ 'name': d.name, @@ -2276,7 +2275,7 @@ class Flattener(object): if key not in self._vars: continue # Don't "override" existing vars if it's actually the same value. - if self._vars[key][1] == value: + elif self._vars[key][1] == value: continue # Anything else is overriding a default value from the DEPS. self._vars[key] = (hierarchy + ' [custom_var override]', value) diff --git a/gclient_eval.py b/gclient_eval.py index 11141d118..c0098e4d4 100644 --- a/gclient_eval.py +++ b/gclient_eval.py @@ -38,8 +38,8 @@ class ConstantString(object): def __eq__(self, other): if isinstance(other, ConstantString): return self.value == other.value - - return self.value == other + else: + return self.value == other def __hash__(self): return self.value.__hash__() @@ -304,14 +304,13 @@ def _gclient_eval(node_or_string, filename='', vars_dict=None): raise ValueError( '%s takes exactly one argument (file %r, line %s)' % ( node.func.id, filename, getattr(node, 'lineno', ''))) - if node.func.id == 'Str': if isinstance(node.args[0], ast.Str): return ConstantString(node.args[0].s) raise ValueError('Passed a non-string to Str() (file %r, line%s)' % ( filename, getattr(node, 'lineno', ''))) - - arg = _convert(node.args[0]) + else: + arg = _convert(node.args[0]) if not isinstance(arg, basestring): raise ValueError( 'Var\'s argument must be a variable name (file %r, line %s)' % ( @@ -541,20 +540,16 @@ def EvaluateCondition(condition, variables, referenced_variables=None): def _convert(node, allow_tuple=False): if isinstance(node, ast.Str): return node.s - - if isinstance(node, ast.Tuple) and allow_tuple: + elif isinstance(node, ast.Tuple) and allow_tuple: return tuple(map(_convert, node.elts)) - - if isinstance(node, ast.Name): + elif isinstance(node, ast.Name): if node.id in referenced_variables: raise ValueError( 'invalid cyclic reference to %r (inside %r)' % ( node.id, condition)) - - if node.id in _allowed_names: + elif node.id in _allowed_names: return _allowed_names[node.id] - - if node.id in variables: + elif node.id in variables: value = variables[node.id] # Allow using "native" types, without wrapping everything in strings. @@ -567,18 +562,16 @@ def EvaluateCondition(condition, variables, referenced_variables=None): variables[node.id], variables, referenced_variables.union([node.id])) - - # Implicitly convert unrecognized names to strings. - # If we want to change this, we'll need to explicitly distinguish - # between arguments for GN to be passed verbatim, and ones to - # be evaluated. - return node.id - - if not sys.version_info[:2] < (3, 4) and isinstance( + else: + # Implicitly convert unrecognized names to strings. + # If we want to change this, we'll need to explicitly distinguish + # between arguments for GN to be passed verbatim, and ones to + # be evaluated. + return node.id + elif not sys.version_info[:2] < (3, 4) and isinstance( node, ast.NameConstant): # Since Python 3.4 return node.value - - if isinstance(node, ast.BoolOp) and isinstance(node.op, ast.Or): + elif isinstance(node, ast.BoolOp) and isinstance(node.op, ast.Or): bool_values = [] for value in node.values: bool_values.append(_convert(value)) @@ -587,8 +580,7 @@ def EvaluateCondition(condition, variables, referenced_variables=None): 'invalid "or" operand %r (inside %r)' % ( bool_values[-1], condition)) return any(bool_values) - - if isinstance(node, ast.BoolOp) and isinstance(node.op, ast.And): + elif isinstance(node, ast.BoolOp) and isinstance(node.op, ast.And): bool_values = [] for value in node.values: bool_values.append(_convert(value)) @@ -597,15 +589,13 @@ def EvaluateCondition(condition, variables, referenced_variables=None): 'invalid "and" operand %r (inside %r)' % ( bool_values[-1], condition)) return all(bool_values) - - if isinstance(node, ast.UnaryOp) and isinstance(node.op, ast.Not): + elif isinstance(node, ast.UnaryOp) and isinstance(node.op, ast.Not): value = _convert(node.operand) if not isinstance(value, bool): raise ValueError( 'invalid "not" operand %r (inside %r)' % (value, condition)) return not value - - if isinstance(node, ast.Compare): + elif isinstance(node, ast.Compare): if len(node.ops) != 1: raise ValueError( 'invalid compare: exactly 1 operator required (inside %r)' % ( @@ -629,10 +619,10 @@ def EvaluateCondition(condition, variables, referenced_variables=None): raise ValueError( 'unexpected operator: %s %s (inside %r)' % ( node.ops[0], ast.dump(node), condition)) - - raise ValueError( - 'unexpected AST node: %s %s (inside %r)' % ( - node, ast.dump(node), condition)) + else: + raise ValueError( + 'unexpected AST node: %s %s (inside %r)' % ( + node, ast.dump(node), condition)) return _convert(main_node) @@ -748,8 +738,7 @@ def SetVar(gclient_dict, var_name, value): def _GetVarName(node): if isinstance(node, ast.Call): return node.args[0].s - - if node.s.endswith('}'): + elif node.s.endswith('}'): last_brace = node.s.rfind('{') return node.s[last_brace+1:-1] return None @@ -891,14 +880,12 @@ def GetRevision(gclient_dict, dep_name): dep = gclient_dict['deps'][dep_name] if dep is None: return None - - if isinstance(dep, basestring): + elif isinstance(dep, basestring): _, _, revision = dep.partition('@') return revision or None - - if isinstance(dep, collections_abc.Mapping) and 'url' in dep: + elif isinstance(dep, collections_abc.Mapping) and 'url' in dep: _, _, revision = dep['url'].partition('@') return revision or None - - raise ValueError( - '%s is not a valid git dependency.' % dep_name) + else: + raise ValueError( + '%s is not a valid git dependency.' % dep_name) diff --git a/gclient_scm.py b/gclient_scm.py index 41e8e926a..d79f07e92 100644 --- a/gclient_scm.py +++ b/gclient_scm.py @@ -165,10 +165,10 @@ class SCMWrapper(object): if actual_remote_url: return (gclient_utils.SplitUrlRevision(actual_remote_url)[0].rstrip('/') == gclient_utils.SplitUrlRevision(self.url)[0].rstrip('/')) - - # This may occur if the self.checkout_path exists but does not contain a - # valid git checkout. - return False + else: + # This may occur if the self.checkout_path exists but does not contain a + # valid git checkout. + return False def _DeleteOrMove(self, force): """Delete the checkout directory or move it out of the way. @@ -381,8 +381,7 @@ class GitWrapper(SCMWrapper): if not target_rev: raise gclient_utils.Error('A target revision for the patch must be given') - - if target_rev.startswith(('refs/heads/', 'refs/branch-heads')): + elif target_rev.startswith(('refs/heads/', 'refs/branch-heads')): # If |target_rev| is in refs/heads/** or refs/branch-heads/**, try first # to find the corresponding remote ref for it, since |target_rev| might # point to a local ref which is not up to date with the corresponding @@ -782,18 +781,16 @@ class GitWrapper(SCMWrapper): printed_path=printed_path, merge=False) printed_path = True break - - if re.match(r'quit|q', action, re.I): + elif re.match(r'quit|q', action, re.I): raise gclient_utils.Error("Can't fast-forward, please merge or " "rebase manually.\n" "cd %s && git " % self.checkout_path + "rebase %s" % upstream_branch) - - if re.match(r'skip|s', action, re.I): + elif re.match(r'skip|s', action, re.I): self.Print('Skipping %s' % self.relpath) return - - self.Print('Input not recognized') + else: + self.Print('Input not recognized') elif re.match(b"error: Your local changes to '.*' would be " b"overwritten by merge. Aborting.\nPlease, commit your " b"changes or stash them before you can merge.\n", @@ -1140,18 +1137,16 @@ class GitWrapper(SCMWrapper): # Should this be recursive? rebase_output = scm.GIT.Capture(rebase_cmd, cwd=self.checkout_path) break - - if re.match(r'quit|q', rebase_action, re.I): + elif re.match(r'quit|q', rebase_action, re.I): raise gclient_utils.Error("Please merge or rebase manually\n" "cd %s && git " % self.checkout_path + "%s" % ' '.join(rebase_cmd)) - - if re.match(r'show|s', rebase_action, re.I): + elif re.match(r'show|s', rebase_action, re.I): self.Print('%s' % e.stderr.decode('utf-8').strip()) continue - - gclient_utils.Error("Input not recognized") - continue + else: + gclient_utils.Error("Input not recognized") + continue elif re.search(br'^CONFLICT', e.stdout, re.M): raise gclient_utils.Error("Conflict while rebasing this branch.\n" "Fix the conflict and run gclient again.\n" @@ -1564,12 +1559,15 @@ class CipdWrapper(SCMWrapper): CIPD packages should be reverted at the root by running `CipdRoot.run('revert')`. """ + pass def diff(self, options, args, file_list): """CIPD has no notion of diffing.""" + pass def pack(self, options, args, file_list): """CIPD has no notion of diffing.""" + pass def revinfo(self, options, args, file_list): """Grab the instance ID.""" @@ -1599,3 +1597,4 @@ class CipdWrapper(SCMWrapper): CIPD packages should be updated at the root by running `CipdRoot.run('update')`. """ + pass diff --git a/gclient_utils.py b/gclient_utils.py index 3087ed200..ae958cde5 100644 --- a/gclient_utils.py +++ b/gclient_utils.py @@ -301,8 +301,8 @@ def rmtree(path): exitcode = subprocess.call(['cmd.exe', '/c', 'rd', '/q', '/s', path]) if exitcode == 0: return - - print('rd exited with code %d' % exitcode, file=sys.stderr) + else: + print('rd exited with code %d' % exitcode, file=sys.stderr) time.sleep(3) raise Exception('Failed to remove path %s' % path) @@ -437,12 +437,11 @@ class Annotated(Wrapper): lf_loc = obj[0].find(b'\n') if cr_loc == lf_loc == -1: break - - if cr_loc == -1 or (0 <= lf_loc < cr_loc): + elif cr_loc == -1 or (lf_loc >= 0 and lf_loc < cr_loc): line, remaining = obj[0].split(b'\n', 1) - if line: - self._wrapped_write(b'%d>%s\n' % (index, line)) - elif lf_loc == -1 or (0 <= cr_loc < lf_loc): + if line: + self._wrapped_write(b'%d>%s\n' % (index, line)) + elif lf_loc == -1 or (cr_loc >= 0 and cr_loc < lf_loc): line, remaining = obj[0].split(b'\r', 1) if line: self._wrapped_write(b'%d>%s\r' % (index, line)) @@ -751,16 +750,12 @@ def GetMacWinAixOrLinux(): """Returns 'mac', 'win', or 'linux', matching the current platform.""" if sys.platform.startswith(('cygwin', 'win')): return 'win' - - if sys.platform.startswith('linux'): + elif sys.platform.startswith('linux'): return 'linux' - - if sys.platform == 'darwin': + elif sys.platform == 'darwin': return 'mac' - - if sys.platform.startswith('aix'): + elif sys.platform.startswith('aix'): return 'aix' - raise Error('Unknown platform: ' + sys.platform) @@ -811,6 +806,7 @@ class WorkItem(object): def run(self, work_queue): """work_queue is passed as keyword argument so it should be the last parameters of the function when you override it.""" + pass @property def name(self): @@ -1215,8 +1211,8 @@ def DefaultDeltaBaseCacheLimit(): """ if platform.architecture()[0].startswith('64'): return '2g' - - return '512m' + else: + return '512m' def DefaultIndexPackConfig(url=''): @@ -1263,15 +1259,13 @@ def freeze(obj): """ if isinstance(obj, collections_abc.Mapping): return FrozenDict((freeze(k), freeze(v)) for k, v in obj.items()) - - if isinstance(obj, (list, tuple)): + elif isinstance(obj, (list, tuple)): return tuple(freeze(i) for i in obj) - - if isinstance(obj, set): + elif isinstance(obj, set): return frozenset(freeze(i) for i in obj) - - hash(obj) - return obj + else: + hash(obj) + return obj class FrozenDict(collections_abc.Mapping): diff --git a/gerrit_util.py b/gerrit_util.py index 429e1deba..4d5e78b41 100644 --- a/gerrit_util.py +++ b/gerrit_util.py @@ -254,8 +254,8 @@ class CookiesAuthenticator(Authenticator): if a[0]: secret = base64.b64encode(('%s:%s' % (a[0], a[2])).encode('utf-8')) return 'Basic %s' % secret.decode('utf-8') - - return 'Bearer %s' % a[2] + else: + return 'Bearer %s' % a[2] return None def get_auth_email(self, host): @@ -696,8 +696,7 @@ def GetChangeReview(host, change, revision=None): jmsg = GetChangeRevisions(host, change) if not jmsg: return None - - if len(jmsg) > 1: + elif len(jmsg) > 1: raise GerritError(200, 'Multiple changes found for ChangeId %s.' % change) revision = jmsg[0]['current_revision'] path = 'changes/%s/revisions/%s/review' @@ -982,8 +981,7 @@ def ResetReviewLabels(host, change, label, value='0', message=None, if not jmsg: raise GerritError( 200, 'Could not get review information for change "%s"' % change) - - if jmsg[0]['current_revision'] != revision: + elif jmsg[0]['current_revision'] != revision: raise GerritError(200, 'While resetting labels on change "%s", ' 'a new patchset was uploaded.' % change) @@ -1002,7 +1000,7 @@ def CreateChange(host, project, branch='main', subject='', params=()): """ path = 'changes/' body = {'project': project, 'branch': branch, 'subject': subject} - body.update(dict(params)) + body.update({k: v for k, v in params}) for key in 'project', 'branch', 'subject': if not body[key]: raise GerritError(200, '%s is required' % key.title()) diff --git a/git_cache.py b/git_cache.py index d612c25b8..f544c5abe 100755 --- a/git_cache.py +++ b/git_cache.py @@ -116,7 +116,7 @@ class Mirror(object): def __init__(self, url, refs=None, commits=None, print_func=None): self.url = url - self.fetch_specs = {self.parse_fetch_spec(ref) for ref in (refs or [])} + self.fetch_specs = set([self.parse_fetch_spec(ref) for ref in (refs or [])]) self.fetch_commits = set(commits or []) self.basedir = self.UrlToCacheDir(url) self.mirror_path = os.path.join(self.GetCachePath(), self.basedir) @@ -576,7 +576,8 @@ class Mirror(object): if not prev_dest_prefix: return for path in ls_out_set: - if path in (prev_dest_prefix + '/', prev_dest_prefix + '.ready'): + if (path == prev_dest_prefix + '/' or + path == prev_dest_prefix + '.ready'): continue if path.endswith('.ready'): gsutil.call('rm', path) diff --git a/git_cl.py b/git_cl.py index 74f04d9ce..22c158532 100755 --- a/git_cl.py +++ b/git_cl.py @@ -638,7 +638,7 @@ def _FindYapfConfigFile(fpath, yapf_config_cache, top_dir=None): yapf_file = os.path.join(fpath, YAPF_CONFIG_FILENAME) if os.path.isfile(yapf_file): ret = yapf_file - elif fpath in (top_dir, parent_dir): + elif fpath == top_dir or parent_dir == fpath: # If we're at the top level directory, or if we're at root # there is no provided style. ret = None @@ -1720,18 +1720,18 @@ class Changelist(object): if not force: confirm_or_exit('If you know what you are doing', action='continue') return - - missing = ( - ([] if gerrit_auth else [self._gerrit_host]) + - ([] if git_auth else [git_host])) - DieWithError('Credentials for the following hosts are required:\n' - ' %s\n' - 'These are read from %s (or legacy %s)\n' - '%s' % ( - '\n '.join(missing), - cookie_auth.get_gitcookies_path(), - cookie_auth.get_netrc_path(), - cookie_auth.get_new_password_message(git_host))) + else: + missing = ( + ([] if gerrit_auth else [self._gerrit_host]) + + ([] if git_auth else [git_host])) + DieWithError('Credentials for the following hosts are required:\n' + ' %s\n' + 'These are read from %s (or legacy %s)\n' + '%s' % ( + '\n '.join(missing), + cookie_auth.get_gitcookies_path(), + cookie_auth.get_netrc_path(), + cookie_auth.get_new_password_message(git_host))) def EnsureCanUploadPatchset(self, force): if not self.GetIssue(): @@ -1820,9 +1820,9 @@ class Changelist(object): if m.get('author', {}).get('_account_id') == owner: # Most recent message was by owner. return 'waiting' - - # Some reply from non-owner. - return 'reply' + else: + # Some reply from non-owner. + return 'reply' # Somehow there are no messages even though there are reviewers. return 'unsent' @@ -1845,9 +1845,9 @@ class Changelist(object): data = self._GetChangeDetail(['ALL_REVISIONS']) patchset = data['revisions'][data['current_revision']]['_number'] - dry_run = {int(m['_revision_number']) - for m in data.get('messages', []) - if m.get('tag', '').endswith('dry-run')} + dry_run = set([int(m['_revision_number']) + for m in data.get('messages', []) + if m.get('tag', '').endswith('dry-run')]) for revision_info in sorted(data.get('revisions', {}).values(), key=lambda c: c['_number'], reverse=True): @@ -2634,8 +2634,8 @@ class Changelist(object): if git_footers.get_footer_change_id(new_log_desc): print('git-cl: Added Change-Id to commit message.') return new_log_desc - - DieWithError('ERROR: Gerrit commit-msg hook not installed.') + else: + DieWithError('ERROR: Gerrit commit-msg hook not installed.') def CannotTriggerTryJobReason(self): try: @@ -3341,10 +3341,10 @@ def CMDbaseurl(parser, args): print('Current base-url:') return RunGit(['config', 'branch.%s.base-url' % branch], error_ok=False).strip() - - print('Setting base-url to %s' % args[0]) - return RunGit(['config', 'branch.%s.base-url' % branch, args[0]], - error_ok=False).strip() + else: + print('Setting base-url to %s' % args[0]) + return RunGit(['config', 'branch.%s.base-url' % branch, args[0]], + error_ok=False).strip() def color_for_status(status): @@ -3605,15 +3605,13 @@ def CMDarchive(parser, args): if options.dry_run: print('\nNo changes were made (dry run).\n') return 0 - - if any(branch == current_branch for branch, _ in proposal): + elif any(branch == current_branch for branch, _ in proposal): print('You are currently on a branch \'%s\' which is associated with a ' 'closed codereview issue, so archive cannot proceed. Please ' 'checkout another branch and run this command again.' % current_branch) return 1 - - if not options.force: + elif not options.force: answer = gclient_utils.AskForData('\nProceed with deletion (Y/n)? ').lower() if answer not in ('y', ''): print('Aborted.') @@ -4150,9 +4148,7 @@ def GetTargetRef(remote, remote_branch, target_branch): if not match: # This is a branch path but not one we recognize; use as-is. remote_branch = target_branch - # pylint: disable=consider-using-get elif remote_branch in REFS_THAT_ALIAS_TO_OTHER_REFS: - # pylint: enable=consider-using-get # Handle the refs that need to land in different refs. remote_branch = REFS_THAT_ALIAS_TO_OTHER_REFS[remote_branch] @@ -4569,10 +4565,8 @@ def GetTreeStatus(url=None): status = str(urllib.request.urlopen(url).read().lower()) if status.find('closed') != -1 or status == '0': return 'closed' - - if status.find('open') != -1 or status == '1': + elif status.find('open') != -1 or status == '1': return 'open' - return 'unknown' return 'unset' @@ -5108,8 +5102,8 @@ def _RunRustFmt(opts, rust_diff_files, top_dir, upstream_commit): if opts.presubmit and rustfmt_exitcode != 0: return 2 - - return 0 + else: + return 0 def MatchingFileType(file_name, extensions): diff --git a/git_common.py b/git_common.py index 0a984aeb8..54e65e82e 100644 --- a/git_common.py +++ b/git_common.py @@ -46,7 +46,6 @@ from io import BytesIO if sys.version_info.major == 2: # On Python 3, BrokenPipeError is raised instead. - # pylint:disable=redefined-builtin BrokenPipeError = IOError @@ -875,7 +874,7 @@ def status(): while c != b'': c = stream.read(1) if c in (None, b'', b'\0'): - if len(acc.getvalue()) > 0: + if len(acc.getvalue()): yield acc.getvalue() acc = BytesIO() else: diff --git a/git_footers.py b/git_footers.py index a096b6f5c..766a958c5 100755 --- a/git_footers.py +++ b/git_footers.py @@ -71,8 +71,7 @@ def split_footers(message): for line in reversed(message_lines): if line == '' or line.isspace(): break - - if parse_footer(line): + elif parse_footer(line): footer_lines.extend(maybe_footer_lines) maybe_footer_lines = [] footer_lines.append(line) @@ -183,8 +182,8 @@ def get_unique(footers, key): assert len(values) <= 1, 'Multiple %s footers' % key if values: return values[0] - - return None + else: + return None def get_position(footers): diff --git a/git_map.py b/git_map.py index d7514a469..e8d4513d9 100755 --- a/git_map.py +++ b/git_map.py @@ -31,7 +31,6 @@ from third_party import colorama if sys.version_info.major == 2: # On Python 3, BrokenPipeError is raised instead. - # pylint:disable=redefined-builtin BrokenPipeError = IOError @@ -69,7 +68,7 @@ def _print_help(outbuf): def _color_branch(branch, all_branches, all_tags, current): - if branch in (current, 'HEAD -> ' + current): + if branch == current or branch == 'HEAD -> ' + current: color = CYAN current = None elif branch in all_branches: diff --git a/git_nav_downstream.py b/git_nav_downstream.py index 85f9855dc..1eeeb6ab0 100755 --- a/git_nav_downstream.py +++ b/git_nav_downstream.py @@ -41,8 +41,7 @@ def main(args): if not downstreams: print("No downstream branches") return 1 - - if len(downstreams) == 1: + elif len(downstreams) == 1: run('checkout', downstreams[0], stdout=sys.stdout, stderr=sys.stderr) else: high = len(downstreams) - 1 diff --git a/git_number.py b/git_number.py index f46b1eeb6..12528335a 100755 --- a/git_number.py +++ b/git_number.py @@ -62,8 +62,8 @@ def pathlify(hash_prefix): """ if sys.version_info.major == 3: return '/'.join('%02x' % b for b in hash_prefix) - - return '/'.join('%02x' % ord(b) for b in hash_prefix) + else: + return '/'.join('%02x' % ord(b) for b in hash_prefix) @git.memoize_one(threadsafe=False) diff --git a/gn.py b/gn.py index 0cf2538d1..52d892dba 100755 --- a/gn.py +++ b/gn.py @@ -66,7 +66,8 @@ def main(args): print( 'gn.py: Could not find gn executable at: %s' % gn_path, file=sys.stderr) return 2 - return subprocess.call([gn_path] + args[1:]) + else: + return subprocess.call([gn_path] + args[1:]) if __name__ == '__main__': diff --git a/metrics_utils.py b/metrics_utils.py index 96982a9d8..7097857a7 100644 --- a/metrics_utils.py +++ b/metrics_utils.py @@ -54,8 +54,7 @@ def get_notice_footer(): def get_change_notice(version): if version == 0: return [] # No changes for version 0 - - if version == 1: + elif version == 1: return [ 'We want to collect the Git version.', 'We want to collect information about the HTTP', @@ -65,8 +64,7 @@ def get_change_notice(version): 'We only collect known strings to make sure we', 'don\'t record PII.', ] - - if version == 2: + elif version == 2: return [ 'We will start collecting metrics from bots.', 'There are no changes for developers.', diff --git a/my_activity.py b/my_activity.py index 7fb0f1cf6..874f9f386 100755 --- a/my_activity.py +++ b/my_activity.py @@ -68,7 +68,7 @@ try: from dateutil.relativedelta import relativedelta except ImportError: logging.error('python-dateutil package required') - sys.exit(1) + exit(1) class DefaultFormatter(Formatter): @@ -76,11 +76,10 @@ class DefaultFormatter(Formatter): super(DefaultFormatter, self).__init__() self.default = default - def get_value(self, key, args, kwargs): - if isinstance(key, str) and key not in kwargs: + def get_value(self, key, args, kwds): + if isinstance(key, str) and key not in kwds: return self.default - return Formatter.get_value(self, key, args, kwargs) - + return Formatter.get_value(self, key, args, kwds) gerrit_instances = [ { @@ -169,10 +168,9 @@ def get_week_of(date): def get_yes_or_no(msg): while True: response = gclient_utils.AskForData(msg + ' yes/no [no] ') - if response in ('y', 'yes'): + if response == 'y' or response == 'yes': return True - - if not response or response in ('n', 'no'): + elif not response or response == 'n' or response == 'no': return False @@ -412,7 +410,7 @@ class MyActivity(object): return [ issue for issue in issues - if user_str in (issue['author'], issue['owner'])] + if issue['author'] == user_str or issue['owner'] == user_str] def monorail_get_issues(self, project, issue_ids): return self.monorail_query_issues(project, { @@ -533,7 +531,7 @@ class MyActivity(object): return True def filter_modified(self, modified): - return self.modified_after < modified < self.modified_before + return self.modified_after < modified and modified < self.modified_before def auth_for_changes(self): #TODO(cjhopman): Move authentication check for getting changes here. @@ -688,12 +686,12 @@ class MyActivity(object): return output class PythonObjectEncoder(json.JSONEncoder): - def default(self, o): # pylint: disable=method-hidden - if isinstance(o, datetime): - return o.isoformat() - if isinstance(o, set): - return list(o) - return json.JSONEncoder.default(self, o) + def default(self, obj): # pylint: disable=method-hidden + if isinstance(obj, datetime): + return obj.isoformat() + if isinstance(obj, set): + return list(obj) + return json.JSONEncoder.default(self, obj) output = { 'reviews': format_for_json_dump(self.reviews), @@ -920,16 +918,16 @@ def main(): config = json.load(f) for item, entries in config.items(): - if item == 'gerrit_instances': - for repo, dic in entries.items(): - # Use property name as URL - dic['url'] = repo - gerrit_instances.append(dic) - elif item == 'monorail_projects': - monorail_projects.append(entries) - else: - logging.error('Invalid entry in config file.') - return 1 + if item == 'gerrit_instances': + for repo, dic in entries.items(): + # Use property name as URL + dic['url'] = repo + gerrit_instances.append(dic) + elif item == 'monorail_projects': + monorail_projects.append(entries) + else: + logging.error('Invalid entry in config file.') + return 1 my_activity = MyActivity(options) my_activity.show_progress('Loading data') diff --git a/owners_finder.py b/owners_finder.py index 98ea7e251..c05aa9c85 100644 --- a/owners_finder.py +++ b/owners_finder.py @@ -95,39 +95,31 @@ class OwnersFinder(object): while True: inp = self.input_command(owner) - if inp in ('y', 'yes'): + if inp == 'y' or inp == 'yes': self.select_owner(owner) break - - if inp in ('n', 'no'): + elif inp == 'n' or inp == 'no': self.deselect_owner(owner) break - - if inp in ('', 'd', 'defer'): + elif inp == '' or inp == 'd' or inp == 'defer': self.owners_queue.append(self.owners_queue.pop(0)) break - - if inp in ('f', 'files'): + elif inp == 'f' or inp == 'files': self.list_files() break - - if inp in ('o', 'owners'): + elif inp == 'o' or inp == 'owners': self.list_owners(self.owners_queue) break - - if inp in ('p', 'pick'): + elif inp == 'p' or inp == 'pick': self.pick_owner(gclient_utils.AskForData('Pick an owner: ')) break - - if inp.startswith('p ') or inp.startswith('pick '): + elif inp.startswith('p ') or inp.startswith('pick '): self.pick_owner(inp.split(' ', 2)[1].strip()) break - - if inp in ('r', 'restart'): + elif inp == 'r' or inp == 'restart': self.reset() break - - if inp in ('q', 'quit'): + elif inp == 'q' or inp == 'quit': # Exit with error return 1 @@ -276,13 +268,11 @@ class OwnersFinder(object): self.writeln('You cannot pick ' + self.bold_name(ow) + ' manually. ' + 'It\'s an invalid name or not related to the change list.') return False - - if ow in self.selected_owners: + elif ow in self.selected_owners: self.writeln('You cannot pick ' + self.bold_name(ow) + ' manually. ' + 'It\'s already selected.') return False - - if ow in self.deselected_owners: + elif ow in self.deselected_owners: self.writeln('You cannot pick ' + self.bold_name(ow) + ' manually.' + 'It\'s already unselected.') return False diff --git a/presubmit_canned_checks.py b/presubmit_canned_checks.py index af207018b..433e35eb2 100644 --- a/presubmit_canned_checks.py +++ b/presubmit_canned_checks.py @@ -70,9 +70,9 @@ def CheckChangeHasBugField(input_api, output_api): 'Buganizer bugs should be prefixed with b:, not b/.') ] return [] - - return [output_api.PresubmitNotifyResult( - 'If this change has an associated bug, add Bug: [bug number].')] + else: + return [output_api.PresubmitNotifyResult( + 'If this change has an associated bug, add Bug: [bug number].')] def CheckChangeHasNoUnwantedTags(input_api, output_api): UNWANTED_TAGS = { @@ -94,13 +94,12 @@ def CheckChangeHasNoUnwantedTags(input_api, output_api): def CheckDoNotSubmitInDescription(input_api, output_api): """Checks that the user didn't add 'DO NOT ''SUBMIT' to the CL description. """ - # Keyword is concatenated to avoid presubmit check rejecting the CL. - keyword = 'DO NOT ' + 'SUBMIT' + keyword = 'DO NOT ''SUBMIT' if keyword in input_api.change.DescriptionText(): return [output_api.PresubmitError( keyword + ' is present in the changelist description.')] - - return [] + else: + return [] def CheckChangeHasDescription(input_api, output_api): @@ -109,8 +108,8 @@ def CheckChangeHasDescription(input_api, output_api): if text.strip() == '': if input_api.is_committing: return [output_api.PresubmitError('Add a description to the CL.')] - - return [output_api.PresubmitNotifyResult('Add a description to the CL.')] + else: + return [output_api.PresubmitNotifyResult('Add a description to the CL.')] return [] @@ -189,9 +188,7 @@ def CheckDoNotSubmitInFiles(input_api, output_api): """Checks that the user didn't add 'DO NOT ''SUBMIT' to any files.""" # We want to check every text file, not just source files. file_filter = lambda x : x - - # Keyword is concatenated to avoid presubmit check rejecting the CL. - keyword = 'DO NOT ' + 'SUBMIT' + keyword = 'DO NOT ''SUBMIT' def DoNotSubmitRule(extension, line): try: return keyword not in line @@ -318,7 +315,7 @@ def CheckGenderNeutral(input_api, output_api, source_file_filter=None): if gendered_re.search(line): errors.append('%s (%d): %s' % (f.LocalPath(), line_num, line)) - if errors: + if len(errors): return [output_api.PresubmitPromptWarning('Found a gendered pronoun in:', long_text='\n'.join(errors))] return [] @@ -529,7 +526,7 @@ def CheckLongLines(input_api, output_api, maxlen, source_file_filter=None): """True iff the pylint directive starting at line[pos] is global.""" # Any character before |pos| that is not whitespace or '#' indidcates # this is a local directive. - return not any(c not in " \t#" for c in line[:pos]) + return not any([c not in " \t#" for c in line[:pos]]) def check_python_long_lines(affected_files, error_formatter): errors = [] @@ -586,8 +583,8 @@ def CheckLongLines(input_api, output_api, maxlen, source_file_filter=None): if errors: msg = 'Found lines longer than %s characters (first 5 shown).' % maxlen return [output_api.PresubmitPromptWarning(msg, items=errors[:5])] - - return [] + else: + return [] def CheckLicense(input_api, output_api, license_re=None, project_name=None, @@ -963,7 +960,7 @@ def GetPylint(input_api, extra_paths_list = extra_paths_list or [] assert version in ('1.5', '2.6', '2.7'), \ - 'Unsupported pylint version: %s' % version + 'Unsupported pylint version: ' + version python2 = (version == '1.5') if input_api.is_committing: @@ -1072,10 +1069,11 @@ def GetPylint(input_api, GetPylintCmd(files, ["--disable=cyclic-import"], True), GetPylintCmd(files, ["--disable=all", "--enable=cyclic-import"], False) ] + else: + return [ GetPylintCmd(files, [], True) ] - return [ GetPylintCmd(files, [], True) ] - - return map(lambda x: GetPylintCmd([x], [], False), files) + else: + return map(lambda x: GetPylintCmd([x], [], 1), files) def RunPylint(input_api, *args, **kwargs): @@ -1092,11 +1090,11 @@ def CheckDirMetadataFormat(input_api, output_api, dirmd_bin=None): # complete. file_filter = lambda f: ( input_api.basename(f.LocalPath()) in ('DIR_METADATA', 'OWNERS')) - affected_files = { + affected_files = set([ f.AbsoluteLocalPath() for f in input_api.change.AffectedFiles( include_deletes=False, file_filter=file_filter) - } + ]) if not affected_files: return [] @@ -1147,11 +1145,11 @@ def CheckOwnersDirMetadataExclusive(input_api, output_api): input_api.re.MULTILINE) file_filter = ( lambda f: input_api.basename(f.LocalPath()) in ('OWNERS', 'DIR_METADATA')) - affected_dirs = { + affected_dirs = set([ input_api.os_path.dirname(f.AbsoluteLocalPath()) for f in input_api.change.AffectedFiles( include_deletes=False, file_filter=file_filter) - } + ]) errors = [] for path in affected_dirs: @@ -1175,11 +1173,11 @@ def CheckOwnersDirMetadataExclusive(input_api, output_api): def CheckOwnersFormat(input_api, output_api): if input_api.gerrit and input_api.gerrit.IsCodeOwnersEnabledOnRepo(): return [] - affected_files = { + affected_files = set([ f.LocalPath() for f in input_api.change.AffectedFiles() if 'OWNERS' in f.LocalPath() and f.Action() != 'D' - } + ]) if not affected_files: return [] try: @@ -1204,9 +1202,8 @@ def CheckOwners( if input_api.gerrit and input_api.gerrit.IsCodeOwnersEnabledOnRepo(): return [] - affected_files = {f.LocalPath() for f in - input_api.change.AffectedFiles( - file_filter=source_file_filter)} + affected_files = set([f.LocalPath() for f in + input_api.change.AffectedFiles(file_filter=source_file_filter)]) owner_email, reviewers = GetCodereviewOwnerAndReviewers( input_api, approval_needed=input_api.is_committing) @@ -1738,7 +1735,7 @@ def CheckChangedLUCIConfigs(input_api, output_api): sev = msg['severity'] if sev == 'WARNING': out_f = output_api.PresubmitPromptWarning - elif sev in ('ERROR', 'CRITICAL'): + elif sev == 'ERROR' or sev == 'CRITICAL': out_f = output_api.PresubmitError else: out_f = output_api.PresubmitNotifyResult diff --git a/presubmit_canned_checks_test_mocks.py b/presubmit_canned_checks_test_mocks.py index 82dee2475..80ab3407d 100644 --- a/presubmit_canned_checks_test_mocks.py +++ b/presubmit_canned_checks_test_mocks.py @@ -124,7 +124,7 @@ class MockInputApi(object): if file_.LocalPath() == filename: return '\n'.join(file_.NewContents()) # Otherwise, file is not in our mock API. - raise IOError("No such file or directory: '%s'" % filename) + raise IOError, "No such file or directory: '%s'" % filename class MockOutputApi(object): diff --git a/presubmit_support.py b/presubmit_support.py index ed28f7f34..50bfc8d77 100755 --- a/presubmit_support.py +++ b/presubmit_support.py @@ -330,11 +330,9 @@ class _PresubmitResult(object): """ if isinstance(val, str): return val - if six.PY2 and isinstance(val, unicode): return val.encode() - - if six.PY3 and isinstance(val, bytes): + elif six.PY3 and isinstance(val, bytes): return val.decode() raise ValueError("Unknown string type %s" % type(val)) @@ -381,6 +379,7 @@ class _PresubmitPromptWarning(_PresubmitResult): # Public access through OutputApi object. class _PresubmitNotifyResult(_PresubmitResult): """Just print something to the screen -- but it's not even a warning.""" + pass # Top level object so multiprocessing can pickle @@ -1283,7 +1282,7 @@ class Change(object): def owners_file_filter(f): return 'OWNERS' in os.path.split(f.LocalPath())[1] files = self.AffectedFiles(file_filter=owners_file_filter) - return {f.LocalPath(): f.OldContents() for f in files} + return dict([(f.LocalPath(), f.OldContents()) for f in files]) class GitChange(Change): @@ -1314,7 +1313,7 @@ def ListRelevantPresubmitFiles(files, root): files = [normpath(os.path.join(root, f)) for f in files] # List all the individual directories containing files. - directories = {os.path.dirname(f) for f in files} + directories = set([os.path.dirname(f) for f in files]) # Ignore root if inherit-review-settings-ok is present. if os.path.isfile(os.path.join(root, 'inherit-review-settings-ok')): @@ -1747,7 +1746,7 @@ def DoPresubmitChecks(change, global _ASKED_FOR_FEEDBACK # Ask for feedback one time out of 5. - if (results and random.randint(0, 4) == 0 and not _ASKED_FOR_FEEDBACK): + if (len(results) and random.randint(0, 4) == 0 and not _ASKED_FOR_FEEDBACK): sys.stdout.write( 'Was the presubmit check useful? If not, run "git cl presubmit -v"\n' 'to figure out which PRESUBMIT.py was run, then run git blame\n' diff --git a/scm.py b/scm.py index 8368bd010..39f8c58ec 100644 --- a/scm.py +++ b/scm.py @@ -71,23 +71,23 @@ def determine_scm(root): """ if os.path.isdir(os.path.join(root, '.git')): return 'git' - - try: - subprocess2.check_call( - ['git', 'rev-parse', '--show-cdup'], - stdout=subprocess2.DEVNULL, - stderr=subprocess2.DEVNULL, - cwd=root) - return 'git' - except (OSError, subprocess2.CalledProcessError): - return None + else: + try: + subprocess2.check_call( + ['git', 'rev-parse', '--show-cdup'], + stdout=subprocess2.DEVNULL, + stderr=subprocess2.DEVNULL, + cwd=root) + return 'git' + except (OSError, subprocess2.CalledProcessError): + return None def only_int(val): if val.isdigit(): return int(val) - - return 0 + else: + return 0 class GIT(object): @@ -261,8 +261,7 @@ class GIT(object): if 'origin/main' in remote_branches: # Fall back on origin/main if it exits. return 'origin', 'refs/heads/main' - - if 'origin/master' in remote_branches: + elif 'origin/master' in remote_branches: # Fall back on origin/master if it exits. return 'origin', 'refs/heads/master' diff --git a/subcommand.py b/subcommand.py index f77a8e4ee..4087627ab 100644 --- a/subcommand.py +++ b/subcommand.py @@ -147,11 +147,11 @@ class CommandDispatcher(object): reverse=True) if (hamming_commands[0][0] - hamming_commands[1][0]) < 0.3: # Too ambiguous. - return None + return if hamming_commands[0][0] < 0.8: # Not similar enough. Don't be a fool and run a random command. - return None + return return commands[hamming_commands[0][1]] diff --git a/testing_support/fake_repos.py b/testing_support/fake_repos.py index 8794ec01e..ba2441c6a 100755 --- a/testing_support/fake_repos.py +++ b/testing_support/fake_repos.py @@ -49,7 +49,7 @@ def read_tree(tree_root): dirs.remove(d) for f in [join(root, f) for f in files if not f.startswith('.')]: filepath = f[len(tree_root) + 1:].replace(os.sep, '/') - assert len(filepath) > 0, f + assert len(filepath), f with io.open(join(root, f), encoding='utf-8') as f: tree[filepath] = f.read() return tree diff --git a/tests/bot_update_coverage_test.py b/tests/bot_update_coverage_test.py index 3bf8ab373..47329f01d 100755 --- a/tests/bot_update_coverage_test.py +++ b/tests/bot_update_coverage_test.py @@ -9,6 +9,8 @@ import os import sys import unittest +#import test_env # pylint: disable=relative-import,unused-import + sys.path.insert(0, os.path.join( os.path.dirname(os.path.dirname(os.path.abspath(__file__))), 'recipes', 'recipe_modules', 'bot_update', 'resources')) diff --git a/tests/download_from_google_storage_unittest.py b/tests/download_from_google_storage_unittest.py index eb3ab32bf..78e09f2d1 100755 --- a/tests/download_from_google_storage_unittest.py +++ b/tests/download_from_google_storage_unittest.py @@ -59,8 +59,8 @@ class GsutilMock(object): if fn: fn() return code - - return 0 + else: + return 0 def check_call(self, *args): with self.lock: @@ -70,8 +70,8 @@ class GsutilMock(object): if fn: fn() return code, out, err - - return (0, '', '') + else: + return (0, '', '') def check_call_with_retries(self, *args): return self.check_call(*args) diff --git a/tests/gclient_scm_test.py b/tests/gclient_scm_test.py index f9a60f159..fbb15c3fc 100755 --- a/tests/gclient_scm_test.py +++ b/tests/gclient_scm_test.py @@ -165,10 +165,10 @@ from :3 return self.OptionsObject(*args, **kwargs) def checkstdout(self, expected): - # pylint: disable=no-member value = sys.stdout.getvalue() sys.stdout.close() # Check that the expected output appears. + # pylint: disable=no-member self.assertIn(expected, strip_timestamps(value)) @staticmethod @@ -597,10 +597,10 @@ class ManagedGitWrapperTestCaseMock(unittest.TestCase): return self.OptionsObject(*args, **kwargs) def checkstdout(self, expected): - # pylint: disable=no-member value = sys.stdout.getvalue() sys.stdout.close() # Check that the expected output appears. + # pylint: disable=no-member self.assertIn(expected, strip_timestamps(value)) def setUp(self): @@ -711,15 +711,15 @@ class ManagedGitWrapperTestCaseMock(unittest.TestCase): class UnmanagedGitWrapperTestCase(BaseGitWrapperTestCase): def checkInStdout(self, expected): - # pylint: disable=no-member value = sys.stdout.getvalue() sys.stdout.close() + # pylint: disable=no-member self.assertIn(expected, value) def checkNotInStdout(self, expected): - # pylint: disable=no-member value = sys.stdout.getvalue() sys.stdout.close() + # pylint: disable=no-member self.assertNotIn(expected, value) def getCurrentBranch(self): diff --git a/tests/gclient_smoketest_base.py b/tests/gclient_smoketest_base.py index fcb137f4d..6b992d72f 100644 --- a/tests/gclient_smoketest_base.py +++ b/tests/gclient_smoketest_base.py @@ -143,3 +143,5 @@ class GClientSmokeBase(fake_repos.FakeReposTestBase): self.checkString(results[i][0][2], path, (i, results[i][0][2], path)) self.assertEqual(len(results), len(items), (stdout, items, len(results))) return results + + diff --git a/tests/git_find_releases_test.py b/tests/git_find_releases_test.py index d4d9490da..30440bdd1 100755 --- a/tests/git_find_releases_test.py +++ b/tests/git_find_releases_test.py @@ -42,11 +42,9 @@ class TestGitFindReleases(unittest.TestCase): assert len(args) > 1 if args[0] == 'name-rev' and args[1] == '--tags': return 'undefined' - - if args[0] == 'name-rev' and args[1] == '--refs': + elif args[0] == 'name-rev' and args[1] == '--refs': return '1.0.0' - - if args[0] == 'log': + elif args[0] == 'log': return '' assert False, "Unexpected arguments for git.run" diff --git a/tests/git_migrate_default_branch_test.py b/tests/git_migrate_default_branch_test.py index 977713bda..483c51269 100755 --- a/tests/git_migrate_default_branch_test.py +++ b/tests/git_migrate_default_branch_test.py @@ -53,14 +53,11 @@ class CMDFormatTestCase(unittest.TestCase): def RunMock(*args): if args[0] == 'remote': return 'origin\ngerrit' - - if args[0] == 'fetch': + elif args[0] == 'fetch': return - - if args[0] == 'branch': + elif args[0] == 'branch': return - - if args[0] == 'config': + elif args[0] == 'config': return raise Exception('Did not expect such run git command: %s' % args[0]) diff --git a/tests/presubmit_unittest.py b/tests/presubmit_unittest.py index ede038309..9a4c8e3a0 100755 --- a/tests/presubmit_unittest.py +++ b/tests/presubmit_unittest.py @@ -1261,7 +1261,7 @@ class InputApiUnittest(PresubmitTestsBase): # Ignores weird because of check_list, third_party because of skip_list, # binary isn't a text file and being deleted doesn't exist. The rest is # outside foo/. - rhs_lines = list(input_api.RightHandSideLines(None)) + rhs_lines = [x for x in input_api.RightHandSideLines(None)] self.assertEqual(len(rhs_lines), 14) self.assertEqual(rhs_lines[0][0].LocalPath(), presubmit.normpath(files[0][1])) @@ -2282,8 +2282,8 @@ the current line as well! "print('foo')\n" ) license_text = ( - r".*? Copyright \(c\) 2037 Nobody.\n" - r".*? All Rights Reserved\.\n" + r".*? Copyright \(c\) 2037 Nobody." "\n" + r".*? All Rights Reserved\." "\n" ) self._LicenseCheck(text, license_text, True, None) @@ -2295,8 +2295,8 @@ the current line as well! "print('foo')\n" ) license_text = ( - r".*? Copyright \(c\) 0007 Nobody.\n" - r".*? All Rights Reserved\.\n" + r".*? Copyright \(c\) 0007 Nobody." "\n" + r".*? All Rights Reserved\." "\n" ) self._LicenseCheck(text, license_text, True, presubmit.OutputApi.PresubmitPromptWarning) @@ -2309,8 +2309,8 @@ the current line as well! "print('foo')\n" ) license_text = ( - r".*? Copyright \(c\) 0007 Nobody.\n" - r".*? All Rights Reserved\.\n" + r".*? Copyright \(c\) 0007 Nobody." "\n" + r".*? All Rights Reserved\." "\n" ) self._LicenseCheck(text, license_text, False, presubmit.OutputApi.PresubmitPromptWarning) @@ -2318,8 +2318,8 @@ the current line as well! def testCheckLicenseEmptySuccess(self): text = '' license_text = ( - r".*? Copyright \(c\) 2037 Nobody.\n" - r".*? All Rights Reserved\.\n" + r".*? Copyright \(c\) 2037 Nobody." "\n" + r".*? All Rights Reserved\." "\n" ) self._LicenseCheck(text, license_text, True, None, accept_empty_files=True) @@ -2449,14 +2449,14 @@ the current line as well! subprocess.Popen.return_value = process presubmit.sigint_handler.wait.return_value = (b'', None) - pylint = os.path.join(_ROOT, 'pylint-2.7') + pylint = os.path.join(_ROOT, 'pylint-1.5') pylintrc = os.path.join(_ROOT, 'pylintrc') env = {str('PYTHONPATH'): str('')} if sys.platform == 'win32': pylint += '.bat' results = presubmit_canned_checks.RunPylint( - input_api, presubmit.OutputApi, version='2.7') + input_api, presubmit.OutputApi) self.assertEqual([], results) self.assertEqual(subprocess.Popen.mock_calls, [ diff --git a/tests/subprocess2_test.py b/tests/subprocess2_test.py index e7733ef50..2337f7516 100755 --- a/tests/subprocess2_test.py +++ b/tests/subprocess2_test.py @@ -141,7 +141,6 @@ class SmokeTests(unittest.TestCase): def test_check_output_no_stdout(self): for subp in (subprocess, subprocess2): with self.assertRaises(ValueError): - # pylint: disable=unexpected-keyword-arg subp.check_output(TEST_COMMAND, stdout=subp.PIPE) def test_print_exception(self): diff --git a/upload_to_google_storage.py b/upload_to_google_storage.py index 5529db0aa..9dce2b6fd 100755 --- a/upload_to_google_storage.py +++ b/upload_to_google_storage.py @@ -135,10 +135,10 @@ def get_targets(args, parser, use_null_terminator): # Take stdin as a newline or null separated list of files. if use_null_terminator: return sys.stdin.read().split('\0') - - return sys.stdin.read().splitlines() - - return args + else: + return sys.stdin.read().splitlines() + else: + return args def upload_to_google_storage(