From cb2985fb698f1ed5c9280fc713804e6816c4cfcc Mon Sep 17 00:00:00 2001 From: "maruel@chromium.org" Date: Wed, 3 Nov 2010 14:08:31 +0000 Subject: [PATCH] Largely reduce the number of pylint warnings and fix one typo. Most of them are style issues or variable aliasing. TEST=Can almost enable pylint warnings BUG=none Review URL: http://codereview.chromium.org/4360002 git-svn-id: svn://svn.chromium.org/chrome/trunk/tools/depot_tools@64908 0039d316-1c4b-4281-b951-d872f2087c98 --- breakpad.py | 4 +-- chrome-update.py | 12 +++---- drover.py | 2 +- gcl.py | 2 +- gclient.py | 7 ++--- gclient_scm.py | 1 - gclient_utils.py | 8 ++--- git-try | 2 +- git_cl_hooks.py | 9 +++--- presubmit_canned_checks.py | 6 ++-- presubmit_support.py | 63 ++++++++++++++++++++----------------- pylintrc | 6 +++- tests/gclient_scm_test.py | 9 +++--- tests/gclient_utils_test.py | 2 +- tests/presubmit_unittest.py | 9 +++--- trychange.py | 9 +++--- watchlists.py | 6 ++-- 17 files changed, 82 insertions(+), 75 deletions(-) diff --git a/breakpad.py b/breakpad.py index bcfdb8da9..ef59b551f 100644 --- a/breakpad.py +++ b/breakpad.py @@ -46,9 +46,9 @@ def SendStack(last_tb, stack, url=None): params['exception'] = str(last_tb) except: pass - print '\n'.join(' %s: %s' % (k, v[0:50]) for k,v in params.iteritems()) + print('\n'.join(' %s: %s' % (k, v[0:50]) for k, v in params.iteritems())) request = urllib.urlopen(url, urllib.urlencode(params)) - print request.read() + print(request.read()) request.close() except IOError: print('There was a failure while trying to send the stack trace. Too bad.') diff --git a/chrome-update.py b/chrome-update.py index d1eb1768f..388936178 100755 --- a/chrome-update.py +++ b/chrome-update.py @@ -15,9 +15,9 @@ COMPILE_URL = BASE_URL + 'slave/compile.py' UTILS_URL = BASE_URL + 'common/chromium_utils.py' -def Fetch(url, file): - if not os.path.exists(file): - urllib.urlretrieve(url, file) +def Fetch(url, filename): + if not os.path.exists(filename): + urllib.urlretrieve(url, filename) def GetLastestRevision(): @@ -53,10 +53,10 @@ def DoUpdate(chrome_root): def DoBuild(chrome_root, args): """Download compile.py and run it.""" - compile = os.path.join(chrome_root, 'compile.py') - Fetch(COMPILE_URL, compile) + compile_path = os.path.join(chrome_root, 'compile.py') + Fetch(COMPILE_URL, compile_path) Fetch(UTILS_URL, os.path.join(chrome_root, 'chromium_utils.py')) - cmd = ['python', compile] + args + cmd = ['python', compile_path] + args return subprocess.call(cmd, cwd=chrome_root, shell=IS_WIN) diff --git a/drover.py b/drover.py index d511ee039..96a1724f5 100755 --- a/drover.py +++ b/drover.py @@ -9,7 +9,7 @@ import re import subprocess import sys -import breakpad +import breakpad # pylint: disable=W0611 import gclient_utils diff --git a/gcl.py b/gcl.py index 39f3bec1d..49e4e70f6 100755 --- a/gcl.py +++ b/gcl.py @@ -35,7 +35,7 @@ except ImportError: sys.path.append(os.path.join(os.path.dirname(__file__), 'third_party')) import simplejson as json -import breakpad +import breakpad # pylint: disable=W0611 # gcl now depends on gclient. from scm import SVN diff --git a/gclient.py b/gclient.py index 0d1e2f93f..054f52d93 100644 --- a/gclient.py +++ b/gclient.py @@ -58,22 +58,21 @@ import os import posixpath import pprint import re -import subprocess import sys import urlparse import urllib -import breakpad +import breakpad # pylint: disable=W0611 import gclient_scm import gclient_utils from third_party.repo.progress import Progress -def attr(attr, data): +def attr(attribute, data): """Sets an attribute on a function.""" def hook(fn): - setattr(fn, attr, data) + setattr(fn, attribute, data) return fn return hook diff --git a/gclient_scm.py b/gclient_scm.py index 6f875eaf2..b5e886173 100644 --- a/gclient_scm.py +++ b/gclient_scm.py @@ -8,7 +8,6 @@ import logging import os import posixpath import re -import sys import time import scm diff --git a/gclient_utils.py b/gclient_utils.py index 4c74c5c5e..ecc59cc7f 100644 --- a/gclient_utils.py +++ b/gclient_utils.py @@ -14,7 +14,6 @@ """Generic utils.""" -import copy import errno import logging import os @@ -300,7 +299,8 @@ def SoftClone(obj): """Clones an object. copy.copy() doesn't work on 'file' objects.""" if obj.__class__.__name__ == 'SoftCloned': return obj - class SoftCloned(object): pass + class SoftCloned(object): + pass new_obj = SoftCloned() for member in dir(obj): if member.startswith('_'): @@ -389,7 +389,7 @@ def MakeFileAnnotated(fileobj): try: # Detect threads no longer existing. indexes = (getattr(t, 'index', None) for t in threading.enumerate()) - indexed = filter(None, indexes) + indexes = filter(None, indexes) for index in new_fileobj.output_buffers: if not index in indexes: orphans.append((index, new_fileobj.output_buffers[index][0])) @@ -480,7 +480,7 @@ def FindGclientRoot(from_dir, filename='.gclient'): # might have failed. In that case, we cannot verify that the .gclient # is the one we want to use. In order to not to cause too much trouble, # just issue a warning and return the path anyway. - print >>sys.stderr, ("%s file in parent directory %s might not be the " + print >> sys.stderr, ("%s file in parent directory %s might not be the " "file you want to use" % (filename, path)) return path scope = {} diff --git a/git-try b/git-try index 34ff5981b..be8083af2 100755 --- a/git-try +++ b/git-try @@ -7,7 +7,7 @@ import logging import sys -import breakpad +import breakpad # pylint: disable=W0611 import gclient_utils from scm import GIT diff --git a/git_cl_hooks.py b/git_cl_hooks.py index 4519e422e..1ba66087b 100644 --- a/git_cl_hooks.py +++ b/git_cl_hooks.py @@ -3,11 +3,10 @@ # found in the LICENSE file. import os -import re import subprocess import sys -import breakpad +import breakpad # pylint: disable=W0611 from git_cl_repo import git_cl from git_cl_repo import upload @@ -25,11 +24,11 @@ def Backquote(cmd, cwd=None): cwd=cwd, stdout=subprocess.PIPE).communicate()[0].strip() -def ConvertToInteger(input): +def ConvertToInteger(inputval): """Convert a string to integer, but returns either an int or None.""" try: - return int(input) - except TypeError, ValueError: + return int(inputval) + except (TypeError, ValueError): return None diff --git a/presubmit_canned_checks.py b/presubmit_canned_checks.py index 52d93b4fb..e470dc7c8 100644 --- a/presubmit_canned_checks.py +++ b/presubmit_canned_checks.py @@ -275,11 +275,11 @@ def CheckLongLines(input_api, output_api, maxlen=80, source_file_filter=None): return [] -def CheckLicense(input_api, output_api, license, source_file_filter=None, +def CheckLicense(input_api, output_api, license_re, source_file_filter=None, accept_empty_files=True): """Verifies the license header. """ - license_re = input_api.re.compile(license, input_api.re.MULTILINE) + license_re = input_api.re.compile(license_re, input_api.re.MULTILINE) bad_files = [] for f in input_api.AffectedSourceFiles(source_file_filter): contents = input_api.ReadFile(f, 'rb') @@ -483,7 +483,7 @@ def CheckRietveldTryJobExecution(input_api, output_api, host_url, platforms, for platform in platforms: values.setdefault(platform, ['not started', '']) message = None - non_success = [k.upper() for k,v in values.iteritems() if v[0] != 'success'] + non_success = [k.upper() for k, v in values.iteritems() if v[0] != 'success'] if 'failure' in [v[0] for v in values.itervalues()]: message = 'Try job failures on %s!\n' % ', '.join(non_success) elif non_success: diff --git a/presubmit_support.py b/presubmit_support.py index 8ab01daae..50fabb102 100755 --- a/presubmit_support.py +++ b/presubmit_support.py @@ -1,5 +1,5 @@ #!/usr/bin/python -# Copyright (c) 2006-2009 The Chromium Authors. All rights reserved. +# Copyright (c) 2010 The Chromium Authors. All rights reserved. # Use of this source code is governed by a BSD-style license that can be # found in the LICENSE file. @@ -32,7 +32,7 @@ import traceback # Exposed through the API. import types import unittest # Exposed through the API. import urllib2 # Exposed through the API. -import warnings +from warnings import warn try: import simplejson as json @@ -48,7 +48,6 @@ except ImportError: import simplejson as json # Local imports. -import gcl import gclient_utils import presubmit_canned_checks import scm @@ -75,11 +74,23 @@ def normpath(path): path = path.replace(os.sep, '/') return os.path.normpath(path) + def PromptYesNo(input_stream, output_stream, prompt): output_stream.write(prompt) response = input_stream.readline().strip().lower() return response == 'y' or response == 'yes' + +def _RightHandSideLinesImpl(affected_files): + """Implements RightHandSideLines for InputApi and GclChange.""" + for af in affected_files: + lines = af.NewContents() + line_number = 0 + for line in lines: + line_number += 1 + yield (af, line_number, line) + + class OutputApi(object): """This class (more like a module) gets passed to presubmit scripts so that they can specify various types of results. @@ -158,7 +169,8 @@ class OutputApi(object): class MailTextResult(PresubmitResult): """A warning that should be included in the review request email.""" def __init__(self, *args, **kwargs): - raise NotImplementedException() # TODO(joi) Implement. + super(OutputApi.MailTextResult, self).__init__() + raise NotImplementedException() class InputApi(object): @@ -309,10 +321,10 @@ class InputApi(object): thereof. """ if include_deletes is not None: - warnings.warn("AffectedTextFiles(include_deletes=%s)" - " is deprecated and ignored" % str(include_deletes), - category=DeprecationWarning, - stacklevel=2) + warn("AffectedTextFiles(include_deletes=%s)" + " is deprecated and ignored" % str(include_deletes), + category=DeprecationWarning, + stacklevel=2) return filter(lambda x: x.IsTextFile(), self.AffectedFiles(include_dirs=False, include_deletes=False)) @@ -327,8 +339,8 @@ class InputApi(object): Note: Copy-paste this function to suit your needs or use a lambda function. """ - def Find(affected_file, list): - for item in list: + def Find(affected_file, items): + for item in items: local_path = affected_file.LocalPath() if self.re.match(item, local_path): logging.debug("%s matched %s" % (item, local_path)) @@ -364,7 +376,7 @@ class InputApi(object): Note: The cariage return (LF or CR) is stripped off. """ files = self.AffectedSourceFiles(source_file_filter) - return InputApi._RightHandSideLinesImpl(files) + return _RightHandSideLinesImpl(files) def ReadFile(self, file_item, mode='r'): """Reads an arbitrary file. @@ -377,16 +389,6 @@ class InputApi(object): raise IOError('Access outside the repository root is denied.') return gclient_utils.FileRead(file_item, mode) - @staticmethod - def _RightHandSideLinesImpl(affected_files): - """Implements RightHandSideLines for InputApi and GclChange.""" - for af in affected_files: - lines = af.NewContents() - line_number = 0 - for line in lines: - line_number += 1 - yield (af, line_number, line) - class AffectedFile(object): """Representation of a file in a change.""" @@ -665,10 +667,10 @@ class Change(object): def AffectedTextFiles(self, include_deletes=None): """Return a list of the existing text files in a change.""" if include_deletes is not None: - warnings.warn("AffectedTextFiles(include_deletes=%s)" - " is deprecated and ignored" % str(include_deletes), - category=DeprecationWarning, - stacklevel=2) + warn("AffectedTextFiles(include_deletes=%s)" + " is deprecated and ignored" % str(include_deletes), + category=DeprecationWarning, + stacklevel=2) return filter(lambda x: x.IsTextFile(), self.AffectedFiles(include_dirs=False, include_deletes=False)) @@ -698,9 +700,9 @@ class Change(object): integer line number (1-based); and the contents of the line as a string. """ - return InputApi._RightHandSideLinesImpl( - filter(lambda x: x.IsTextFile(), - self.AffectedFiles(include_deletes=False))) + return _RightHandSideLinesImpl( + x for x in self.AffectedFiles(include_deletes=False) + if x.IsTextFile()) class SvnChange(Change): @@ -716,6 +718,8 @@ class SvnChange(Change): if self._changelists == None: previous_cwd = os.getcwd() os.chdir(self.RepositoryRoot()) + # Need to import here to avoid circular dependency. + import gcl self._changelists = gcl.GetModifiedFiles() os.chdir(previous_cwd) return self._changelists @@ -792,7 +796,8 @@ def ListRelevantPresubmitFiles(files, root): class GetTrySlavesExecuter(object): - def ExecPresubmitScript(self, script_text): + @staticmethod + def ExecPresubmitScript(script_text): """Executes GetPreferredTrySlaves() from a single presubmit script. Args: diff --git a/pylintrc b/pylintrc index bb5f1b987..ca128bfcc 100644 --- a/pylintrc +++ b/pylintrc @@ -36,6 +36,7 @@ load-plugins= # C0111: Missing docstring # C0302: Too many lines in module (N) # I0011: Locally disabling WNNNN +# R0801: Similar lines in N files # R0901: Too many ancestors (8/7) # R0902: Too many instance attributes (N/7) # R0903: Too few public methods (N/2) @@ -46,10 +47,13 @@ load-plugins= # R0915: Too many statements (N/50) # W0122: Use of the exec statement # W0141: Used builtin function '' +# W0142: Used * or ** magic +# W0402: Uses of a deprecated module 'string' +# W0511: TODO # W0603: Using the global statement # W0613: Unused argument '' # W6501: Specify string format arguments as logging function parameters -disable=C0103,C0111,C0302,I0011,R0901,R0902,R0903,R0911,R0912,R0913,R0914,R0915,W0122,W0141,W0603,W0613,W6501 +disable=C0103,C0111,C0302,I0011,R0801,R0901,R0902,R0903,R0911,R0912,R0913,R0914,R0915,W0122,W0141,W0142,W0402,W0511,W0603,W0613,W6501 [REPORTS] diff --git a/tests/gclient_scm_test.py b/tests/gclient_scm_test.py index a075d86c7..f49b2d82b 100755 --- a/tests/gclient_scm_test.py +++ b/tests/gclient_scm_test.py @@ -17,6 +17,7 @@ import __builtin__ # Fixes include path. from super_mox import mox, StdoutCheck, TestCaseUtils, SuperMoxTestBase +import sys import gclient_scm # Shortcut since this function is used often @@ -789,11 +790,11 @@ from :3 end = ('] test\n 1 files changed, 1 insertions(+), ' '1 deletions(-)\n\n_____ . at refs/heads/master\n' 'Attempting rebase onto refs/remotes/origin/master...\n') - self.assertTrue(gclient_scm.sys.stdout.getvalue().startswith(start)) - self.assertTrue(gclient_scm.sys.stdout.getvalue().endswith(end)) - self.assertEquals(len(gclient_scm.sys.stdout.getvalue()), + self.assertTrue(sys.stdout.getvalue().startswith(start)) + self.assertTrue(sys.stdout.getvalue().endswith(end)) + self.assertEquals(len(sys.stdout.getvalue()), len(start) + len(end) + 7) - gclient_scm.sys.stdout.close() + sys.stdout.close() def testUpdateNotGit(self): if not self.enabled: diff --git a/tests/gclient_utils_test.py b/tests/gclient_utils_test.py index 801e23f71..934e4bb99 100755 --- a/tests/gclient_utils_test.py +++ b/tests/gclient_utils_test.py @@ -30,7 +30,7 @@ class GclientUtilsUnittest(GclientUtilBase): 'ParseXML', 'Popen', 'PrintableObject', 'RemoveDirectory', 'SoftClone', 'SplitUrlRevision', 'SyntaxErrorToError', 'WorkItem', - 'copy', 'errno', 'logging', 'os', 'Queue', 're', 'stat', 'subprocess', + 'errno', 'logging', 'os', 'Queue', 're', 'stat', 'subprocess', 'sys','threading', 'time', 'xml', ] # If this test fails, you should add the relevant test. diff --git a/tests/presubmit_unittest.py b/tests/presubmit_unittest.py index f6880025e..8e59e8915 100755 --- a/tests/presubmit_unittest.py +++ b/tests/presubmit_unittest.py @@ -37,7 +37,7 @@ def GetPreferredTrySlaves(): def setUp(self): SuperMoxTestBase.setUp(self) self.mox.StubOutWithMock(presubmit, 'random') - self.mox.StubOutWithMock(presubmit, 'warnings') + self.mox.StubOutWithMock(presubmit, 'warn') presubmit._ASKED_FOR_FEEDBACK = False self.fake_root_dir = self.RootDir() # Special mocks. @@ -69,11 +69,11 @@ class PresubmitUnittest(PresubmitTestsBase): 'NotImplementedException', 'OutputApi', 'ParseFiles', 'PresubmitExecuter', 'PromptYesNo', 'ScanSubDirs', 'SvnAffectedFile', 'SvnChange', 'cPickle', 'cStringIO', - 'exceptions', 'fnmatch', 'gcl', 'gclient_utils', 'glob', 'json', + 'exceptions', 'fnmatch', 'gclient_utils', 'glob', 'json', 'logging', 'marshal', 'normpath', 'optparse', 'os', 'pickle', 'presubmit_canned_checks', 'random', 're', 'scm', 'subprocess', 'sys', 'tempfile', 'time', 'traceback', 'types', 'unittest', 'urllib2', - 'warnings', + 'warn', ] # If this test fails, you should add the relevant test. self.compareMembers(presubmit, members) @@ -898,8 +898,7 @@ class InputApiUnittest(PresubmitTestsBase): normpath(join(self.fake_root_dir, 'isdir', 'blat.cc'))) def testDeprecated(self): - presubmit.warnings.warn(mox.IgnoreArg(), category=mox.IgnoreArg(), - stacklevel=2) + presubmit.warn(mox.IgnoreArg(), category=mox.IgnoreArg(), stacklevel=2) self.mox.ReplayAll() change = presubmit.Change('mychange', '', self.fake_root_dir, [], 0, 0) diff --git a/trychange.py b/trychange.py index 5d73cb71d..d560a78fa 100755 --- a/trychange.py +++ b/trychange.py @@ -2,6 +2,7 @@ # Copyright (c) 2009 The Chromium Authors. All rights reserved. # Use of this source code is governed by a BSD-style license that can be # found in the LICENSE file. + """Client-side script to send a try job to the try server. It communicates to the try server by either writting to a svn repository or by directly connecting to the server by HTTP. @@ -29,7 +30,7 @@ except ImportError: json = None try: - import breakpad + import breakpad # pylint: disable=W0611 except ImportError: pass @@ -205,11 +206,9 @@ class SVN(SCM): return data # Try to search on the subversion repository for the file. - try: - from gcl import GetCachedFile - except ImportError: + if not gcl: return None - data = GetCachedFile(filename) + data = gcl.GetCachedFile(filename) logging.debug('%s:\n%s' % (filename, data)) return data diff --git a/watchlists.py b/watchlists.py index 9d38faf49..57b810cb1 100755 --- a/watchlists.py +++ b/watchlists.py @@ -106,9 +106,11 @@ class Watchlists(object): for path in paths: path = path.replace(os.sep, '/') for name, rule in self._defns.iteritems(): - if name not in self._watchlists: continue + if name not in self._watchlists: + continue rex_str = rule.get('filepath') - if not rex_str: continue + if not rex_str: + continue if re.search(rex_str, path): map(watchers.add, self._watchlists[name]) return list(watchers)