repo-pull: Fix pylint errors

This commit adds several pylint fixes.

Test: for i in *.py; do pylint $i; done
Change-Id: Ie7318b616ef75c2944a4c03185f49b2a3f02db50

tools/repo_pull/gerrit.py

@@ -16,6 +16,8 @@
 # limitations under the License.
 #
 
+"""Gerrit Restful API client library."""
+
 from __future__ import print_function
 
 import argparse
@@ -32,8 +34,10 @@ except ImportError:
         HTTPBasicAuthHandler, Request, build_opener)  # PY2
 
 try:
+    # pylint: disable=ungrouped-imports
     from urllib.parse import urlencode, urlparse  # PY3
 except ImportError:
+    # pylint: disable=ungrouped-imports
     from urllib import urlencode  # PY2
     from urlparse import urlparse  # PY2
 
@@ -41,7 +45,7 @@ except ImportError:
 def load_auth_credentials_from_file(cookie_file):
     """Load credentials from an opened .gitcookies file."""
     credentials = {}
-    for lineno, line in enumerate(cookie_file, start=1):
+    for line in cookie_file:
         if line.startswith('#HttpOnly_'):
             line = line[len('#HttpOnly_'):]
 
@@ -192,6 +196,7 @@ def _parse_args():
 
 
 def main():
+    """Main function"""
     args = _parse_args()
 
     # Query change lists

tools/repo_pull/repo_patch.py

@@ -21,11 +21,11 @@ Gerrit."""
 
 from __future__ import print_function
 
-from gerrit import create_url_opener_from_args, query_change_lists, get_patch
-
 import argparse
 import os
 
+from gerrit import create_url_opener_from_args, query_change_lists, get_patch
+
 
 def _parse_args():
     """Parse command line options."""
@@ -45,6 +45,7 @@ def _parse_args():
 
 
 def main():
+    """Main function"""
     args = _parse_args()
 
     # Query change lists
@@ -57,8 +58,8 @@ def main():
     num_changes_width = len(str(num_changes))
     for i, change in enumerate(change_lists, start=1):
         print('{:>{}}/{} | {} {}'.format(
-                i, num_changes_width, num_changes, change['_number'],
+            i, num_changes_width, num_changes, change['_number'],
-                change['subject']))
+            change['subject']))
 
         patch_file = get_patch(url_opener, args.gerrit, change['id'])
         with open('{}.patch'.format(change['_number']), 'wb') as output_file:

tools/repo_pull/repo_pull.py

@@ -20,8 +20,6 @@
 
 from __future__ import print_function
 
-from gerrit import create_url_opener_from_args, query_change_lists
-
 import argparse
 import collections
 import itertools
@@ -32,7 +30,10 @@ import re
 import sys
 import xml.dom.minidom
 
+from gerrit import create_url_opener_from_args, query_change_lists
+
 try:
+    # pylint: disable=redefined-builtin
     from __builtin__ import raw_input as input  # PY2
 except ImportError:
     pass
@@ -44,9 +45,9 @@ except ImportError:
     # it doesn't have to be quoted.
     _SHELL_SIMPLE_PATTERN = re.compile('^[a-zA-Z90-9_./-]+$')
 
-    def _sh_quote(s):
+    def _sh_quote(txt):
         """Quote a string if it contains special characters."""
-        return s if _SHELL_SIMPLE_PATTERN.match(s) else json.dumps(s)
+        return txt if _SHELL_SIMPLE_PATTERN.match(txt) else json.dumps(txt)
 
 try:
     from subprocess import PIPE, run  # PY3.5
@@ -54,6 +55,9 @@ except ImportError:
     from subprocess import CalledProcessError, PIPE, Popen
 
     class CompletedProcess(object):
+        """Process execution result returned by subprocess.run()."""
+        # pylint: disable=too-few-public-methods
+
         def __init__(self, args, returncode, stdout, stderr):
             self.args = args
             self.returncode = returncode
@@ -61,18 +65,20 @@ except ImportError:
             self.stderr = stderr
 
     def run(*args, **kwargs):
+        """Run a command with subprocess.Popen() and redirect input/output."""
+
         check = kwargs.pop('check', False)
 
         try:
-            input = kwargs.pop('input')
+            stdin = kwargs.pop('input')
             assert 'stdin' not in kwargs
             kwargs['stdin'] = PIPE
         except KeyError:
-            input = None
+            stdin = None
 
         proc = Popen(*args, **kwargs)
         try:
-            stdout, stderr = proc.communicate(input)
+            stdout, stderr = proc.communicate(stdin)
         except:
             proc.kill()
             proc.wait()
@@ -83,18 +89,22 @@ except ImportError:
             raise CalledProcessError(returncode, args, stdout)
         return CompletedProcess(args, returncode, stdout, stderr)
 
+
 if bytes is str:
     def write_bytes(data, file):  # PY2
         """Write bytes to a file."""
+        # pylint: disable=redefined-builtin
         file.write(data)
 else:
     def write_bytes(data, file):  # PY3
         """Write bytes to a file."""
+        # pylint: disable=redefined-builtin
         file.buffer.write(data)
 
 
 def _confirm(question, default, file=sys.stderr):
     """Prompt a yes/no question and convert the answer to a boolean value."""
+    # pylint: disable=redefined-builtin
     answers = {'': default, 'y': True, 'yes': True, 'n': False, 'no': False}
     suffix = '[Y/n] ' if default else ' [y/N] '
     while True:
@@ -107,10 +117,13 @@ def _confirm(question, default, file=sys.stderr):
 
 class ChangeList(object):
     """A ChangeList to be checked out."""
+    # pylint: disable=too-few-public-methods,too-many-instance-attributes
 
     def __init__(self, project, project_dir, fetch, commit_sha1, commit,
                  change_list):
         """Initialize a ChangeList instance."""
+        # pylint: disable=too-many-arguments
+
         self.project = project
         self.project_dir = project_dir
         self.number = change_list['_number']
@@ -154,7 +167,7 @@ def find_manifest_xml(dir_path):
     raise ValueError('.repo dir not found')
 
 
-def build_project_name_to_directory_dict(manifest_path):
+def build_project_name_dir_dict(manifest_path):
     """Build the mapping from Gerrit project name to source tree project
     directory path."""
     project_dirs = {}
@@ -177,10 +190,14 @@ def group_and_sort_change_lists(change_lists, project_dirs):
     # Build a dict that map projects to dicts that map commits to changes.
     projects = collections.defaultdict(dict)
     for change_list in change_lists:
+        commit_sha1 = None
         for commit_sha1, value in change_list['revisions'].items():
             fetch = value['fetch']
             commit = value['commit']
 
+        if not commit_sha1:
+            raise ValueError('bad revision')
+
         project = change_list['project']
         project_dir = project_dirs[project]
 
@@ -197,17 +214,17 @@ def group_and_sort_change_lists(change_lists, project_dirs):
         visited_changes = set()
         sorted_changes = []
 
-        def post_order_traverse(change):
+        def _post_order_traverse(change):
             visited_changes.add(change)
             for parent in change.parents:
                 parent_change = changes.get(parent['commit'])
                 if parent_change and parent_change not in visited_changes:
-                    post_order_traverse(parent_change)
+                    _post_order_traverse(parent_change)
             sorted_changes.append(change)
 
         for change in sorted(changes.values(), key=lambda x: x.number):
             if change not in visited_changes:
-                post_order_traverse(change)
+                _post_order_traverse(change)
 
         return sorted_changes
 
@@ -280,7 +297,7 @@ def _main_bash(args):
     branch_name = _get_local_branch_name_from_args(args)
 
     manifest_path = _get_manifest_xml_from_args(args)
-    project_dirs = build_project_name_to_directory_dict(manifest_path)
+    project_dirs = build_project_name_dir_dict(manifest_path)
 
     change_lists = _get_change_lists_from_args(args)
     change_list_groups = group_and_sort_change_lists(change_lists, project_dirs)
@@ -314,13 +331,32 @@ def _do_pull_change_lists_for_project(task):
     return None
 
 
+def _print_pull_failures(failures, file=sys.stderr):
+    """Print pull failures and tracebacks."""
+    # pylint: disable=redefined-builtin
+
+    separator = '=' * 78
+    separator_sub = '-' * 78
+
+    print(separator, file=file)
+    for failed_change, skipped_changes, cmd, errors in failures:
+        print('PROJECT:', failed_change.project, file=file)
+        print('FAILED COMMIT:', failed_change.commit_sha1, file=file)
+        for change in skipped_changes:
+            print('PENDING COMMIT:', change.commit_sha1, file=file)
+        print(separator_sub, file=sys.stderr)
+        print('FAILED COMMAND:', _sh_quote_command(cmd), file=file)
+        write_bytes(errors, file=sys.stderr)
+        print(separator, file=sys.stderr)
+
+
 def _main_pull(args):
     """Pull the change lists."""
 
     branch_name = _get_local_branch_name_from_args(args)
 
     manifest_path = _get_manifest_xml_from_args(args)
-    project_dirs = build_project_name_to_directory_dict(manifest_path)
+    project_dirs = build_project_name_dir_dict(manifest_path)
 
     # Collect change lists
     change_lists = _get_change_lists_from_args(args)
@@ -343,21 +379,9 @@ def _main_pull(args):
                            zip(change_list_groups, itertools.repeat(task_opts)))
 
     # Print failures and tracebacks
-    failed_results = [result for result in results if result]
+    failures = [result for result in results if result]
-    if failed_results:
+    if failures:
-        _SEPARATOR = '=' * 78
+        _print_pull_failures(failures)
-        _SEPARATOR_SUB = '-' * 78
-
-        print(_SEPARATOR, file=sys.stderr)
-        for failed_change, skipped_changes, cmd, errors in failed_results:
-            print('PROJECT:', failed_change.project, file=sys.stderr)
-            print('FAILED COMMIT:', failed_change.commit_sha1, file=sys.stderr)
-            for change in skipped_changes:
-                print('PENDING COMMIT:', change.commit_sha1, file=sys.stderr)
-            print(_SEPARATOR_SUB, file=sys.stderr)
-            print('FAILED COMMAND:', _sh_quote_command(cmd), file=sys.stderr)
-            write_bytes(errors, file=sys.stderr)
-            print(_SEPARATOR, file=sys.stderr)
         sys.exit(1)
 
 
@@ -422,6 +446,7 @@ def _get_local_branch_name_from_args(args):
 
 
 def main():
+    """Main function"""
     args = _parse_args()
     if args.command == 'json':
         _main_json(args)

tools/repo_pull/repo_review.py

@@ -20,17 +20,17 @@
 
 from __future__ import print_function
 
-from gerrit import create_url_opener_from_args, query_change_lists, set_review
+import argparse
+import json
+import os
+import sys
 
 try:
     from urllib.error import HTTPError  # PY3
 except ImportError:
     from urllib2 import HTTPError  # PY2
 
-import argparse
+from gerrit import create_url_opener_from_args, query_change_lists, set_review
-import json
-import os
-import sys
 
 
 def _get_labels_from_args(args):
@@ -48,6 +48,7 @@ def _get_labels_from_args(args):
     return labels
 
 
+# pylint: disable=redefined-builtin
 def _print_change_lists(change_lists, file=sys.stdout):
     """Print matching change lists for each projects."""
     change_lists = sorted(
@@ -134,8 +135,8 @@ def main():
         try:
             res_code, res_json = set_review(
                 url_opener, args.gerrit, change['id'], labels, args.message)
-        except HTTPError as e:
+        except HTTPError as error:
-            res_code = e.code
+            res_code = error.code
             res_json = None
 
         if res_code != 200: