Run pychecker over most scripts in depot_tools. Catched a few bugs.

TEST=unit tests
BUG=none

Review URL: http://codereview.chromium.org/385007

git-svn-id: svn://svn.chromium.org/chrome/trunk/tools/depot_tools@31590 0039d316-1c4b-4281-b951-d872f2087c98

gclient_scm.py

@@ -96,16 +96,18 @@ class GitWrapper(SCMWrapper):
 
   def cleanup(self, options, args, file_list):
     """Cleanup working copy."""
+    __pychecker__ = 'unusednames=args,file_list,options'
     self._RunGit(['prune'], redirect_stdout=False)
     self._RunGit(['fsck'], redirect_stdout=False)
     self._RunGit(['gc'], redirect_stdout=False)
 
   def diff(self, options, args, file_list):
-    # NOTE: This function does not currently modify file_list.
+    __pychecker__ = 'unusednames=args,file_list,options'
     merge_base = self._RunGit(['merge-base', 'HEAD', 'origin'])
     self._RunGit(['diff', merge_base], redirect_stdout=False)
 
   def export(self, options, args, file_list):
+    __pychecker__ = 'unusednames=file_list,options'
     assert len(args) == 1
     export_path = os.path.abspath(os.path.join(args[0], self.relpath))
     if not os.path.exists(export_path):
@@ -167,6 +169,7 @@ class GitWrapper(SCMWrapper):
 
     All reverted files will be appended to file_list.
     """
+    __pychecker__ = 'unusednames=args'
     path = os.path.join(self._root_dir, self.relpath)
     if not os.path.isdir(path):
       # revert won't work if the directory doesn't exist. It needs to
@@ -181,6 +184,7 @@ class GitWrapper(SCMWrapper):
 
   def revinfo(self, options, args, file_list):
     """Display revision"""
+    __pychecker__ = 'unusednames=args,file_list,options'
     return self._RunGit(['rev-parse', 'HEAD'])
 
   def runhooks(self, options, args, file_list):
@@ -188,9 +192,10 @@ class GitWrapper(SCMWrapper):
 
   def status(self, options, args, file_list):
     """Display status information."""
+    __pychecker__ = 'unusednames=args,options'
     if not os.path.isdir(self.checkout_path):
       print('\n________ couldn\'t run status in %s:\nThe directory '
-            'does not exist.' % checkout_path)
+            'does not exist.' % self.checkout_path)
     else:
       merge_base = self._RunGit(['merge-base', 'HEAD', 'origin'])
       self._RunGit(['diff', '--name-status', merge_base], redirect_stdout=False)
@@ -219,17 +224,20 @@ class SVNWrapper(SCMWrapper):
 
   def cleanup(self, options, args, file_list):
     """Cleanup working copy."""
+    __pychecker__ = 'unusednames=file_list,options'
     command = ['cleanup']
     command.extend(args)
     RunSVN(command, os.path.join(self._root_dir, self.relpath))
 
   def diff(self, options, args, file_list):
     # NOTE: This function does not currently modify file_list.
+    __pychecker__ = 'unusednames=file_list,options'
     command = ['diff']
     command.extend(args)
     RunSVN(command, os.path.join(self._root_dir, self.relpath))
 
   def export(self, options, args, file_list):
+    __pychecker__ = 'unusednames=file_list,options'
     assert len(args) == 1
     export_path = os.path.abspath(os.path.join(args[0], self.relpath))
     try:
@@ -361,6 +369,7 @@ class SVNWrapper(SCMWrapper):
     All reverted files will be appended to file_list, even if Subversion
     doesn't know about them.
     """
+    __pychecker__ = 'unusednames=args'
     path = os.path.join(self._root_dir, self.relpath)
     if not os.path.isdir(path):
       # svn revert won't work if the directory doesn't exist. It needs to
@@ -369,9 +378,9 @@ class SVNWrapper(SCMWrapper):
       # Don't reuse the args.
       return self.update(options, [], file_list)
 
-    for file in CaptureSVNStatus(path):
-      file_path = os.path.join(path, file[1])
-      if file[0][0] == 'X':
+    for file_status in CaptureSVNStatus(path):
+      file_path = os.path.join(path, file_status[1])
+      if file_status[0][0] == 'X':
         # Ignore externals.
         logging.info('Ignoring external %s' % file_path)
         continue
@@ -380,7 +389,7 @@ class SVNWrapper(SCMWrapper):
         logging.info('%s%s' % (file[0], file[1]))
       else:
         print(file_path)
-      if file[0].isspace():
+      if file_status[0].isspace():
         logging.error('No idea what is the status of %s.\n'
                       'You just found a bug in gclient, please ping '
                       'maruel@chromium.org ASAP!' % file_path)
@@ -413,6 +422,7 @@ class SVNWrapper(SCMWrapper):
 
   def revinfo(self, options, args, file_list):
     """Display revision"""
+    __pychecker__ = 'unusednames=args,file_list,options'
     return CaptureSVNHeadRevision(self.url)
 
   def runhooks(self, options, args, file_list):
@@ -435,6 +445,7 @@ class SVNWrapper(SCMWrapper):
   def pack(self, options, args, file_list):
     """Generates a patch file which can be applied to the root of the
     repository."""
+    __pychecker__ = 'unusednames=file_list,options'
     path = os.path.join(self._root_dir, self.relpath)
     command = ['diff']
     command.extend(args)
@@ -757,13 +768,12 @@ def CaptureSVNStatus(files):
   if dom:
     # /status/target/entry/(wc-status|commit|author|date)
     for target in dom.getElementsByTagName('target'):
-      base_path = target.getAttribute('path')
       for entry in target.getElementsByTagName('entry'):
-        file = entry.getAttribute('path')
+        file_path = entry.getAttribute('path')
         wc_status = entry.getElementsByTagName('wc-status')
         assert len(wc_status) == 1
         # Emulate svn 1.5 status ouput...
-        statuses = [' ' for i in range(7)]
+        statuses = [' '] * 7
         # Col 0
         xml_item_status = wc_status[0].getAttribute('item')
         if xml_item_status in status_letter:
@@ -793,6 +803,6 @@ def CaptureSVNStatus(files):
         if wc_status[0].getAttribute('switched') == 'true':
           statuses[4] = 'S'
         # TODO(maruel): Col 5 and 6
-        item = (''.join(statuses), file)
+        item = (''.join(statuses), file_path)
         results.append(item)
   return results

presubmit_canned_checks.py

@@ -262,11 +262,11 @@ def CheckSvnProperty(input_api, output_api, prop, expected, affected_files):
   bad = filter(lambda f: f.Property(prop) != expected, affected_files)
   if bad:
     if input_api.is_committing:
-      type = output_api.PresubmitError
+      res_type = output_api.PresubmitError
     else:
-      type = output_api.PresubmitNotifyResult
+      res_type = output_api.PresubmitNotifyResult
     message = "Run `svn pset %s %s <item>` on these files:" % (prop, expected)
-    return [type(message, items=bad)]
+    return [res_type(message, items=bad)]
   return []
 
 

presubmit_support.py

@@ -345,16 +345,16 @@ class InputApi(object):
     files = self.AffectedSourceFiles(source_file_filter)
     return InputApi._RightHandSideLinesImpl(files)
 
-  def ReadFile(self, file, mode='r'):
+  def ReadFile(self, file_item, mode='r'):
     """Reads an arbitrary file.
 
     Deny reading anything outside the repository.
     """
-    if isinstance(file, AffectedFile):
-      file = file.AbsoluteLocalPath()
-    if not file.startswith(self.change.RepositoryRoot()):
+    if isinstance(file_item, AffectedFile):
+      file_item = file_item.AbsoluteLocalPath()
+    if not file_item.startswith(self.change.RepositoryRoot()):
       raise IOError('Access outside the repository root is denied.')
-    return gcl.ReadFile(file, mode)
+    return gcl.ReadFile(file_item, mode)
 
   @staticmethod
   def _RightHandSideLinesImpl(affected_files):
@@ -989,7 +989,7 @@ def ScanSubDirs(mask, recursive):
 def ParseFiles(args, recursive):
   files = []
   for arg in args:
-    files.extend([('M', file) for file in ScanSubDirs(arg, recursive)])
+    files.extend([('M', f) for f in ScanSubDirs(arg, recursive)])
   return files
 
 

tests/gcl_unittest.py

@@ -58,7 +58,7 @@ class GclUnittest(GclTestsBase):
       'UnknownFiles', 'UploadCL', 'Warn', 'WriteFile',
       'gclient_scm', 'gclient_utils', 'getpass', 'main', 'os', 'random', 're',
       'shutil', 'string', 'subprocess', 'sys', 'tempfile',
-      'upload', 'urllib2', 'xml',
+      'upload', 'urllib2',
     ]
     # If this test fails, you should add the relevant test.
     self.compareMembers(gcl, members)

tests/gclient_test.py

@@ -50,7 +50,7 @@ class BaseTestCase(super_mox.SuperMoxTestBase):
   def assertRaisesError(self, msg, fn, *args, **kwargs):
     try:
       fn(*args, **kwargs)
-    except gclient.Error, e:
+    except gclient_utils.Error, e:
       self.assertEquals(e.args[0], msg)
     else:
       self.fail('%s not raised' % msg)
@@ -69,8 +69,8 @@ class GClientBaseTestCase(BaseTestCase):
     self.mox.StubOutWithMock(gclient.sys, 'stdout')
     self.mox.StubOutWithMock(gclient_utils, 'subprocess')
     # These are not tested.
-    self.mox.StubOutWithMock(gclient, 'FileRead')
-    self.mox.StubOutWithMock(gclient, 'FileWrite')
+    self.mox.StubOutWithMock(gclient_utils, 'FileRead')
+    self.mox.StubOutWithMock(gclient_utils, 'FileWrite')
     self.mox.StubOutWithMock(gclient_utils, 'SubprocessCall')
     self.mox.StubOutWithMock(gclient_utils, 'RemoveDirectory')
     # Mock them to be sure nothing bad happens.
@@ -353,7 +353,8 @@ class GClientClassTestCase(GclientTestCase):
   def testSetConfig_ConfigContent_GetVar_SaveConfig_SetDefaultConfig(self):
     options = self.Options()
     text = "# Dummy content\nclient = 'my client'"
-    gclient.FileWrite(os.path.join(self.root_dir, options.config_filename),
+    gclient_utils.FileWrite(
+        os.path.join(self.root_dir, options.config_filename),
         text)
 
     self.mox.ReplayAll()
@@ -429,13 +430,15 @@ class GClientClassTestCase(GclientTestCase):
     # Then an update will be performed.
     scm_wrapper_sol.RunCommand('update', options, self.args, [])
     # Then an attempt will be made to read its DEPS file.
-    gclient.FileRead(os.path.join(self.root_dir,
+    gclient_utils.FileRead(os.path.join(self.root_dir,
                                         solution_name,
-                     options.deps_file)).AndRaise(IOError(2, 'No DEPS file'))
+                                        options.deps_file)
+                           ).AndRaise(IOError(2, 'No DEPS file'))
 
     # After everything is done, an attempt is made to write an entries
     # file.
-    gclient.FileWrite(os.path.join(self.root_dir, options.entries_filename),
+    gclient_utils.FileWrite(
+        os.path.join(self.root_dir, options.entries_filename),
         IsOneOf((entries_content1, entries_content2)))
 
     self.mox.ReplayAll()
@@ -485,7 +488,7 @@ class GClientClassTestCase(GclientTestCase):
     # Then an update will be performed.
     scm_wrapper_sol.RunCommand('update', options, self.args, [])
     # Then an attempt will be made to read its DEPS file.
-    gclient.FileRead(os.path.join(self.root_dir,
+    gclient_utils.FileRead(os.path.join(self.root_dir,
                                         solution_name,
                                         options.deps_file)).AndReturn(deps)
 
@@ -500,7 +503,8 @@ class GClientClassTestCase(GclientTestCase):
 
     # After everything is done, an attempt is made to write an entries
     # file.
-    gclient.FileWrite(os.path.join(self.root_dir, options.entries_filename),
+    gclient_utils.FileWrite(
+        os.path.join(self.root_dir, options.entries_filename),
         entries_content)
 
     self.mox.ReplayAll()
@@ -558,7 +562,7 @@ class GClientClassTestCase(GclientTestCase):
     # Then an update will be performed.
     scm_wrapper_sol.RunCommand('update', options, self.args, [])
     # Then an attempt will be made to read its DEPS file.
-    gclient.FileRead(os.path.join(checkout_path, options.deps_file)
+    gclient_utils.FileRead(os.path.join(checkout_path, options.deps_file)
         ).AndReturn(deps)
 
     # Next we expect an scm to be request for dep src/n even though it does not
@@ -580,7 +584,8 @@ class GClientClassTestCase(GclientTestCase):
 
     # After everything is done, an attempt is made to write an entries
     # file.
-    gclient.FileWrite(os.path.join(self.root_dir, options.entries_filename),
+    gclient_utils.FileWrite(
+        os.path.join(self.root_dir, options.entries_filename),
         entries_content)
 
     self.mox.ReplayAll()
@@ -643,7 +648,8 @@ class GClientClassTestCase(GclientTestCase):
     gclient_scm.CreateSCM(url_a, self.root_dir, name_a).AndReturn(
         scm_wrapper_a)
     # Then an attempt will be made to read it's DEPS file.
-    gclient.FileRead(os.path.join(self.root_dir, name_a, options.deps_file)
+    gclient_utils.FileRead(
+        os.path.join(self.root_dir, name_a, options.deps_file)
         ).AndReturn(deps_a)
     # Then an update will be performed.
     scm_wrapper_a.RunCommand('update', options, self.args, [])
@@ -652,7 +658,8 @@ class GClientClassTestCase(GclientTestCase):
     gclient_scm.CreateSCM(url_b, self.root_dir, name_b).AndReturn(
         scm_wrapper_b)
     # Then an attempt will be made to read its DEPS file.
-    gclient.FileRead(os.path.join(self.root_dir, name_b, options.deps_file)
+    gclient_utils.FileRead(
+        os.path.join(self.root_dir, name_b, options.deps_file)
         ).AndReturn(deps_b)
     # Then an update will be performed.
     scm_wrapper_b.RunCommand('update', options, self.args, [])
@@ -664,7 +671,8 @@ class GClientClassTestCase(GclientTestCase):
     scm_wrapper_dep.RunCommand('update', options, self.args, [])
 
     # After everything is done, an attempt is made to write an entries file.
-    gclient.FileWrite(os.path.join(self.root_dir, options.entries_filename),
+    gclient_utils.FileWrite(
+        os.path.join(self.root_dir, options.entries_filename),
         entries_content)
 
     self.mox.ReplayAll()
@@ -694,9 +702,10 @@ class GClientClassTestCase(GclientTestCase):
     gclient_scm.CreateSCM(self.url, self.root_dir, name).AndReturn(
         gclient_scm.CreateSCM)
     gclient_scm.CreateSCM.RunCommand('update', options, self.args, [])
-    gclient.FileRead(os.path.join(self.root_dir, name, options.deps_file)
+    gclient_utils.FileRead(os.path.join(self.root_dir, name, options.deps_file)
         ).AndReturn("Boo = 'a'")
-    gclient.FileWrite(os.path.join(self.root_dir, options.entries_filename),
+    gclient_utils.FileWrite(
+        os.path.join(self.root_dir, options.entries_filename),
         entries_content)
 
     self.mox.ReplayAll()
@@ -768,9 +777,11 @@ deps_os = {
 
     # Also, pymox doesn't verify the order of function calling w.r.t. different
     # mock objects. Pretty lame. So reorder as we wish to make it clearer.
-    gclient.FileRead(os.path.join(self.root_dir, 'src', options.deps_file)
+    gclient_utils.FileRead(
+        os.path.join(self.root_dir, 'src', options.deps_file)
         ).AndReturn(deps_content)
-    gclient.FileWrite(os.path.join(self.root_dir, options.entries_filename),
+    gclient_utils.FileWrite(
+        os.path.join(self.root_dir, options.entries_filename),
         entries_content)
 
     gclient.os.path.exists(os.path.join(self.root_dir, 'src', '.git')
@@ -853,7 +864,7 @@ deps_os = {
     exception = "Conflicting revision numbers specified."
     try:
       client.RunOnDeps('update', self.args)
-    except gclient.Error, e:
+    except gclient_utils.Error, e:
       self.assertEquals(e.args[0], exception)
     else:
       self.fail('%s not raised' % exception)
@@ -887,9 +898,11 @@ deps = {
     scm_wrapper_src = self.mox.CreateMockAnything()
 
     options = self.Options()
-    gclient.FileRead(os.path.join(self.root_dir, name, options.deps_file)
+    gclient_utils.FileRead(
+        os.path.join(self.root_dir, name, options.deps_file)
         ).AndReturn(deps_content)
-    gclient.FileWrite(os.path.join(self.root_dir, options.entries_filename),
+    gclient_utils.FileWrite(
+        os.path.join(self.root_dir, options.entries_filename),
         entries_content)
 
     gclient.os.path.exists(os.path.join(self.root_dir, 'foo/third_party/WebKit',
@@ -945,9 +958,11 @@ deps = {
     scm_wrapper_src = self.mox.CreateMockAnything()
 
     options = self.Options()
-    gclient.FileRead(os.path.join(self.root_dir, name, options.deps_file)
+    gclient_utils.FileRead(
+        os.path.join(self.root_dir, name, options.deps_file)
         ).AndReturn(deps_content)
-    gclient.FileWrite(os.path.join(self.root_dir, options.entries_filename),
+    gclient_utils.FileWrite(
+        os.path.join(self.root_dir, options.entries_filename),
         entries_content)
 
     gclient.os.path.exists(os.path.join(self.root_dir, 'foo/third_party/WebKit',
@@ -990,7 +1005,8 @@ deps = {
 }"""
 
     options = self.Options()
-    gclient.FileRead(os.path.join(self.root_dir, name, options.deps_file)
+    gclient_utils.FileRead(
+        os.path.join(self.root_dir, name, options.deps_file)
         ).AndReturn(deps_content)
     gclient_scm.CreateSCM(self.url, self.root_dir, name).AndReturn(
         gclient_scm.CreateSCM)
@@ -1002,7 +1018,7 @@ deps = {
     exception = "Var is not defined: webkit"
     try:
       client.RunOnDeps('update', self.args)
-    except gclient.Error, e:
+    except gclient_utils.Error, e:
       self.assertEquals(e.args[0], exception)
     else:
       self.fail('%s not raised' % exception)

tests/trychange_unittest.py

@@ -29,9 +29,9 @@ class TryChangeUnittest(TryChangeTestsBase):
       'GetTryServerSettings', 'GuessVCS',
       'HELP_STRING', 'InvalidScript', 'NoTryServerAccess', 'PathDifference',
       'RunCommand', 'SCM', 'SVN', 'TryChange', 'USAGE',
-      'datetime', 'gcl', 'gclient', 'gclient_scm', 'getpass', 'logging',
+      'datetime', 'gcl', 'gclient_scm', 'getpass', 'logging',
       'optparse', 'os', 'presubmit_support', 'shutil', 'socket', 'subprocess',
-      'sys', 'tempfile', 'traceback', 'upload', 'urllib',
+      'sys', 'tempfile', 'upload', 'urllib',
     ]
     # If this test fails, you should add the relevant test.
     self.compareMembers(trychange, members)

trychange.py

@@ -18,11 +18,9 @@ import socket
 import subprocess
 import sys
 import tempfile
-import traceback
 import urllib
 
 import gcl
-import gclient
 import gclient_scm
 import presubmit_support
 import upload
@@ -134,7 +132,7 @@ class SCM(object):
     self.options = options
 
   def ProcessOptions(self):
-    raise Unimplemented
+    raise NotImplementedError
 
 
 class SVN(SCM):
@@ -153,11 +151,11 @@ class SVN(SCM):
       os.chdir(root)
 
     diff = []
-    for file in files:
+    for filename in files:
       # Use svn info output instead of os.path.isdir because the latter fails
       # when the file is deleted.
-      if gclient_scm.CaptureSVNInfo(file).get("Node Kind") in ("dir",
-                                                               "directory"):
+      if gclient_scm.CaptureSVNInfo(filename).get("Node Kind") in (
+          "dir", "directory"):
         continue
       # If the user specified a custom diff command in their svn config file,
       # then it'll be used when we do svn diff, which we don't want to happen
@@ -173,26 +171,26 @@ class SVN(SCM):
       if not os.path.exists(bogus_dir):
         os.mkdir(bogus_dir)
       # Grabs the diff data.
-      data = gcl.RunShell(["svn", "diff", "--config-dir", bogus_dir, file])
+      data = gcl.RunShell(["svn", "diff", "--config-dir", bogus_dir, filename])
 
       # We know the diff will be incorrectly formatted. Fix it.
-      if gcl.IsSVNMoved(file):
+      if gcl.IsSVNMoved(filename):
         # The file is "new" in the patch sense. Generate a homebrew diff.
         # We can't use ReadFile() since it's not using binary mode.
-        file_handle = open(file, 'rb')
+        file_handle = open(filename, 'rb')
         file_content = file_handle.read()
         file_handle.close()
         # Prepend '+' to every lines.
         file_content = ['+' + i for i in file_content.splitlines(True)]
         nb_lines = len(file_content)
         # We need to use / since patch on unix will fail otherwise.
-        file = file.replace('\\', '/')
-        data = "Index: %s\n" % file
+        filename = filename.replace('\\', '/')
+        data = "Index: %s\n" % filename
         data += ("============================================================="
                  "======\n")
         # Note: Should we use /dev/null instead?
-        data += "--- %s\n" % file
-        data += "+++ %s\n" % file
+        data += "--- %s\n" % filename
+        data += "+++ %s\n" % filename
         data += "@@ -0,0 +1,%d @@\n" % nb_lines
         data += ''.join(file_content)
       diff.append(data)
@@ -255,7 +253,8 @@ class GIT(SCM):
     # patches?
     branch = upload.RunShell(['git', 'symbolic-ref', 'HEAD']).strip()
     if not branch.startswith('refs/heads/'):
-      raise "Couldn't figure out branch name"
+      # TODO(maruel): Find a better type.
+      raise NoTryServerAccess("Couldn't figure out branch name")
     branch = branch[len('refs/heads/'):]
     return branch
 
@@ -377,14 +376,14 @@ def _SendChangeSVN(options):
       # no-op if the file's content (the diff) is not modified. This is why the
       # file name contains the date and time.
       RunCommand(['svn', 'update', full_path])
-      file = open(full_path, 'wb')
-      file.write(options.diff)
-      file.close()
+      f = open(full_path, 'wb')
+      f.write(options.diff)
+      f.close()
     else:
       # Add the file to the repo
-      file = open(full_path, 'wb')
-      file.write(options.diff)
-      file.close()
+      f = open(full_path, 'wb')
+      f.write(options.diff)
+      f.close()
       RunCommand(["svn", "add", full_path])
     temp_file.write(description)
     temp_file.flush()
@@ -572,6 +571,7 @@ def TryChange(argv,
     except NoTryServerAccess, e:
       # If we got the diff, we don't care.
       if not options.diff:
+        # TODO(maruel): Raise what?
         raise
 
     # Get try slaves from PRESUBMIT.py files if not specified.
@@ -588,7 +588,7 @@ def TryChange(argv,
 
     if options.name is None:
       if options.issue:
-        patch_name = 'Issue %s' % options.issue
+        options.name = 'Issue %s' % options.issue
       else:
         options.name = 'Unnamed'
         print('Note: use --name NAME to change the try job name.')