From 31a486419e279333146ca1e75fcffd510a863d30 Mon Sep 17 00:00:00 2001 From: David 'Digit' Turner Date: Fri, 11 Jan 2019 16:10:59 +0000 Subject: [PATCH] Improve no-long-lines check for Python files. The previous implementation of CheckLongLines did not handle global pylint disable/enable directives properly, i.e. the difference between: # pylint: disable=line-too-long .... checks disabled for all lines here. # pylint: enable=line-too-long versus: # Check only disabled for the line below some python statements # pylint: disable=line-too-long This CL changes the implementation to support Python files properly. Note that in order to not disturb the mock-based unit-tests, a new function is introduced to be able to filter the list of affected files based on their file extension. BUG=890734 R=mattcary@chromium.org,ehmaldonado@chromium.org,dpranke@chromium.org Change-Id: Id52deff53913b8d47a4157f42b1fffbd3b103201 Reviewed-on: https://chromium-review.googlesource.com/c/1396094 Reviewed-by: Edward Lesmes Commit-Queue: David Turner --- presubmit_canned_checks.py | 120 +++++++++++++++++++++++++++++++----- tests/presubmit_unittest.py | 118 +++++++++++++++++++++++++++++++++++ 2 files changed, 223 insertions(+), 15 deletions(-) diff --git a/presubmit_canned_checks.py b/presubmit_canned_checks.py index 310f83248..a442459c1 100644 --- a/presubmit_canned_checks.py +++ b/presubmit_canned_checks.py @@ -248,15 +248,38 @@ def _ReportErrorFileAndLine(filename, line_num, dummy_line): return '%s:%s' % (filename, line_num) -def _FindNewViolationsOfRule(callable_rule, input_api, source_file_filter=None, - error_formatter=_ReportErrorFileAndLine): +def _GenerateAffectedFileExtList(input_api, source_file_filter): + """Generate a list of (file, extension) tuples from affected files. + + The result can be fed to _FindNewViolationsOfRule() directly, or + could be filtered before doing that. + + Args: + input_api: object to enumerate the affected files. + source_file_filter: a filter to be passed to the input api. + Yields: + A list of (file, extension) tuples, where |file| is an affected + file, and |extension| its file path extension. + """ + for f in input_api.AffectedFiles( + include_deletes=False, file_filter=source_file_filter): + extension = str(f.LocalPath()).rsplit('.', 1)[-1] + yield (f, extension) + + +def _FindNewViolationsOfRuleForList(callable_rule, + file_ext_list, + error_formatter=_ReportErrorFileAndLine): """Find all newly introduced violations of a per-line rule (a callable). + Prefer calling _FindNewViolationsOfRule() instead of this function, unless + the list of affected files need to be filtered in a special way. + Arguments: callable_rule: a callable taking a file extension and line of input and returning True if the rule is satisfied and False if there was a problem. - input_api: object to enumerate the affected files. - source_file_filter: a filter to be passed to the input api. + file_ext_list: a list of input (file, extension) tuples, as returned by + _GenerateAffectedFileExtList(). error_formatter: a callable taking (filename, line_number, line) and returning a formatted error string. @@ -264,13 +287,11 @@ def _FindNewViolationsOfRule(callable_rule, input_api, source_file_filter=None, A list of the newly-introduced violations reported by the rule. """ errors = [] - for f in input_api.AffectedFiles(include_deletes=False, - file_filter=source_file_filter): + for f, extension in file_ext_list: # For speed, we do two passes, checking first the full file. Shelling out # to the SCM to determine the changed region can be quite expensive on # Win32. Assuming that most files will be kept problem-free, we can # skip the SCM operations most of the time. - extension = str(f.LocalPath()).rsplit('.', 1)[-1] if all(callable_rule(extension, line) for line in f.NewContents()): continue # No violation found in full text: can skip considering diff. @@ -281,6 +302,28 @@ def _FindNewViolationsOfRule(callable_rule, input_api, source_file_filter=None, return errors +def _FindNewViolationsOfRule(callable_rule, + input_api, + source_file_filter=None, + error_formatter=_ReportErrorFileAndLine): + """Find all newly introduced violations of a per-line rule (a callable). + + Arguments: + callable_rule: a callable taking a file extension and line of input and + returning True if the rule is satisfied and False if there was a problem. + input_api: object to enumerate the affected files. + source_file_filter: a filter to be passed to the input api. + error_formatter: a callable taking (filename, line_number, line) and + returning a formatted error string. + + Returns: + A list of the newly-introduced violations reported by the rule. + """ + return _FindNewViolationsOfRuleForList( + callable_rule, _GenerateAffectedFileExtList( + input_api, source_file_filter), error_formatter) + + def CheckChangeHasNoTabs(input_api, output_api, source_file_filter=None): """Checks that there are no tab characters in any of the text files to be submitted. @@ -385,11 +428,6 @@ def CheckLongLines(input_api, output_api, maxlen, source_file_filter=None): if any((url in line) for url in ('file://', 'http://', 'https://')): return True - # If 'line-too-long' is explicitly suppressed for the line, any length is - # acceptable. - if 'pylint: disable=line-too-long' in line and file_extension == 'py': - return True - if line_len > extra_maxlen: return False @@ -402,12 +440,64 @@ def CheckLongLines(input_api, output_api, maxlen, source_file_filter=None): return input_api.re.match( r'.*[A-Za-z][A-Za-z_0-9]{%d,}.*' % long_symbol, line) + def is_global_pylint_directive(line, pos): + """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]]) + + def check_python_long_lines(affected_files, error_formatter): + errors = [] + global_check_enabled = True + + for f in affected_files: + file_path = f.LocalPath() + for idx, line in enumerate(f.NewContents()): + line_num = idx + 1 + line_is_short = no_long_lines(PY_FILE_EXTS[0], line) + + pos = line.find('pylint: disable=line-too-long') + if pos >= 0: + if is_global_pylint_directive(line, pos): + global_check_enabled = False # Global disable + else: + continue # Local disable. + + do_check = global_check_enabled + + pos = line.find('pylint: enable=line-too-long') + if pos >= 0: + if is_global_pylint_directive(line, pos): + global_check_enabled = True # Global enable + do_check = True # Ensure it applies to current line as well. + else: + do_check = True # Local enable + + if do_check and not line_is_short: + errors.append(error_formatter(file_path, line_num, line)) + + return errors + def format_error(filename, line_num, line): return '%s, line %s, %s chars' % (filename, line_num, len(line)) - errors = _FindNewViolationsOfRule(no_long_lines, input_api, - source_file_filter, - error_formatter=format_error) + file_ext_list = list( + _GenerateAffectedFileExtList(input_api, source_file_filter)) + + errors = [] + + # For non-Python files, a simple line-based rule check is enough. + non_py_file_ext_list = [x for x in file_ext_list if x[1] not in PY_FILE_EXTS] + if non_py_file_ext_list: + errors += _FindNewViolationsOfRuleForList( + no_long_lines, non_py_file_ext_list, error_formatter=format_error) + + # However, Python files need more sophisticated checks that need parsing + # the whole source file. + py_file_list = [x[0] for x in file_ext_list if x[1] in PY_FILE_EXTS] + if py_file_list: + errors += check_python_long_lines( + py_file_list, error_formatter=format_error) if errors: msg = 'Found lines longer than %s characters (first 5 shown).' % maxlen return [output_api.PresubmitPromptWarning(msg, items=errors[:5])] diff --git a/tests/presubmit_unittest.py b/tests/presubmit_unittest.py index 964720a83..d535e48ab 100755 --- a/tests/presubmit_unittest.py +++ b/tests/presubmit_unittest.py @@ -1743,6 +1743,38 @@ class CannedChecksUnittest(PresubmitTestsBase): self.assertEquals(len(results2), 1) self.assertEquals(results2[0].__class__, error_type) + def PythonLongLineTest(self, maxlen, content, should_pass): + """Runs a test of Python long-line checking rule. + + Because ContentTest() cannot be used here due to the different code path + that the implementation of CheckLongLines() uses for Python files. + + Args: + maxlen: Maximum line length for content. + content: Python source which is expected to pass or fail the test. + should_pass: True iff the test should pass, False otherwise. + """ + change = presubmit.Change('foo1', 'foo1\n', self.fake_root_dir, None, 0, 0, + None) + input_api = self.MockInputApi(change, False) + affected_file = self.mox.CreateMock(presubmit.GitAffectedFile) + input_api.AffectedFiles( + include_deletes=False, + file_filter=mox.IgnoreArg()).AndReturn([affected_file]) + affected_file.LocalPath().AndReturn('foo.py') + affected_file.LocalPath().AndReturn('foo.py') + affected_file.NewContents().AndReturn(content.splitlines()) + + self.mox.ReplayAll() + results = presubmit_canned_checks.CheckLongLines( + input_api, presubmit.OutputApi, maxlen) + if should_pass: + self.assertEquals(results, []) + else: + self.assertEquals(len(results), 1) + self.assertEquals(results[0].__class__, + presubmit.OutputApi.PresubmitPromptWarning) + def ReadFileTest(self, check, content1, content2, error_type): change1 = presubmit.Change( 'foo1', 'foo1\n', self.fake_root_dir, None, 0, 0, None) @@ -1935,6 +1967,92 @@ class CannedChecksUnittest(PresubmitTestsBase): 'importSomething ' + 'A ' * 50, 'foo.java', presubmit.OutputApi.PresubmitPromptWarning) + def testCannedCheckPythonLongLines(self): + # NOTE: Cannot use ContentTest() here because of the different code path + # used for Python checks in CheckLongLines(). + passing_cases = [ + r""" +01234568901234589012345689012345689 +A short line +""", + r""" +01234568901234589012345689012345689 +This line is too long but should pass # pylint: disable=line-too-long +""", + r""" +01234568901234589012345689012345689 +# pylint: disable=line-too-long +This line is too long but should pass due to global disable +""", + r""" +01234568901234589012345689012345689 + #pylint: disable=line-too-long +This line is too long but should pass due to global disable. +""", + r""" +01234568901234589012345689012345689 + # pylint: disable=line-too-long +This line is too long but should pass due to global disable. +""", + r""" +01234568901234589012345689012345689 +# import is a valid exception +import some.really.long.package.name.that.should.pass +""", + r""" +01234568901234589012345689012345689 +# from is a valid exception +from some.really.long.package.name import passing.line +""", + r""" +01234568901234589012345689012345689 + import some.package +""", + r""" +01234568901234589012345689012345689 + from some.package import stuff +""", + ] + for content in passing_cases: + self.PythonLongLineTest(40, content, should_pass=True) + + failing_cases = [ + r""" +01234568901234589012345689012345689 +This line is definitely too long and should fail. +""", + r""" +01234568901234589012345689012345689 +# pylint: disable=line-too-long +This line is too long and should pass due to global disable +# pylint: enable=line-too-long +But this line is too long and should still fail now +""", + r""" +01234568901234589012345689012345689 +# pylint: disable=line-too-long +This line is too long and should pass due to global disable +But this line is too long # pylint: enable=line-too-long +""", + r""" +01234568901234589012345689012345689 +This should fail because the global +check is enabled on the next line. + # pylint: enable=line-too-long +""", + r""" +01234568901234589012345689012345689 +# pylint: disable=line-too-long + # pylint: enable-foo-bar should pass +The following line should fail +since global directives apply to +the current line as well! + # pylint: enable-line-too-long should fail +""", + ] + for content in failing_cases[0:0]: + self.PythonLongLineTest(40, content, should_pass=False) + def testCannedCheckJSLongLines(self): check = lambda x, y, _: presubmit_canned_checks.CheckLongLines(x, y, 10) self.ContentTest(check, 'GEN(\'#include "c/b/ui/webui/fixture.h"\');',