From 58f2a947db98bdecb061133a7b6780fa405fbd33 Mon Sep 17 00:00:00 2001 From: "Adam J. Stewart" Date: Tue, 25 Apr 2017 13:01:25 -0500 Subject: Properly ignore flake8 F811 redefinition errors (#3932) * Properly ignore flake8 F811 redefinition errors * Add unit tests for flake8 command * Allow spack flake8 to work on systems with older git * Skip flake8 unit tests for Python 2.6 and 3.3 --- lib/spack/spack/cmd/flake8.py | 91 +++++++++++++++++++++++------------- lib/spack/spack/test/cmd/flake8.py | 95 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 153 insertions(+), 33 deletions(-) create mode 100644 lib/spack/spack/test/cmd/flake8.py (limited to 'lib') diff --git a/lib/spack/spack/cmd/flake8.py b/lib/spack/spack/cmd/flake8.py index 546a27c765..33c06155ae 100644 --- a/lib/spack/spack/cmd/flake8.py +++ b/lib/spack/spack/cmd/flake8.py @@ -37,8 +37,6 @@ import spack from spack.util.executable import * description = "runs source code style checks on Spack. requires flake8" -flake8 = None -include_untracked = True """List of directories to exclude from checks.""" exclude_directories = [spack.external_path] @@ -53,24 +51,34 @@ exemptions = { # exemptions applied only to package.py files. r'package.py$': { # Exempt lines with urls and descriptions from overlong line errors. - 501: [r'^\s*homepage\s*=', - r'^\s*url\s*=', - r'^\s*git\s*=', - r'^\s*svn\s*=', - r'^\s*hg\s*=', - r'^\s*version\(.*\)', - r'^\s*variant\(.*\)', - r'^\s*depends_on\(.*\)', - r'^\s*extends\(.*\)', - r'^\s*patch\(.*\)'], + 'E501': [ + r'^\s*homepage\s*=', + r'^\s*url\s*=', + r'^\s*git\s*=', + r'^\s*svn\s*=', + r'^\s*hg\s*=', + r'^\s*list_url\s*=', + r'^\s*version\(.*\)', + r'^\s*variant\(.*\)', + r'^\s*provides\(.*\)', + r'^\s*extends\(.*\)', + r'^\s*depends_on\(.*\)', + r'^\s*conflicts\(.*\)', + r'^\s*resource\(.*\)', + r'^\s*patch\(.*\)', + ], # Exempt '@when' decorated functions from redefinition errors. - 811: [r'^\s*\@when\(.*\)'], + 'F811': [ + r'^\s*@when\(.*\)', + ], }, # exemptions applied to all files. r'.py$': { # Exempt lines with URLs from overlong line errors. - 501: [r'(https?|ftp|file)\:'] + 'E501': [ + r'(https?|ftp|file)\:', + ] }, } @@ -81,7 +89,7 @@ exemptions = dict((re.compile(file_pattern), for file_pattern, error_dict in exemptions.items()) -def changed_files(): +def changed_files(args): """Get list of changed files in the Spack repository.""" git = which('git', required=True) @@ -92,23 +100,30 @@ def changed_files(): # Add changed files that have been staged but not yet committed ['diff', '--name-only', '--diff-filter=ACMR', '--cached'], # Add changed files that are unstaged - ['diff', '--name-only', '--diff-filter=ACMR']] + ['diff', '--name-only', '--diff-filter=ACMR'], + ] # Add new files that are untracked - if include_untracked: + if args.untracked: git_args.append(['ls-files', '--exclude-standard', '--other']) excludes = [os.path.realpath(f) for f in exclude_directories] changed = set() - for git_arg_list in git_args: - arg_list = git_arg_list + ['--', '*.py'] - files = [f for f in git(*arg_list, output=str).split('\n') if f] + for arg_list in git_args: + files = git(*arg_list, output=str).split('\n') + for f in files: - # don't look at files that are in the exclude locations + # Ignore non-Python files + if not f.endswith('.py'): + continue + + # Ignore files in the exclude locations if any(os.path.realpath(f).startswith(e) for e in excludes): continue + changed.add(f) + return sorted(changed) @@ -120,7 +135,17 @@ def filter_file(source, dest, output=False): with open(dest, 'w') as outfile: for line in infile: - line = line.rstrip() + # Only strip newline characters + # We still want to catch trailing whitespace warnings + line = line.rstrip('\n') + + if line == '# flake8: noqa': + # Entire file is ignored + break + + if line.endswith('# noqa'): + # Line is already ignored + continue for file_pattern, errors in exemptions.items(): if not file_pattern.search(source): @@ -129,7 +154,10 @@ def filter_file(source, dest, output=False): for code, patterns in errors.items(): for pattern in patterns: if pattern.search(line): - line += (" # NOQA: E%d" % code) + if '# noqa: ' in line: + line += ',{0}'.format(code) + else: + line += ' # noqa: {0}'.format(code) break oline = line + '\n' @@ -157,10 +185,7 @@ def setup_parser(subparser): def flake8(parser, args): - # Just use this to check for flake8 -- we actually execute it with Popen. - global flake8, include_untracked flake8 = which('flake8', required=True) - include_untracked = args.untracked temp = tempfile.mkdtemp() try: @@ -174,7 +199,7 @@ def flake8(parser, args): with working_dir(spack.prefix): if not file_list: - file_list = changed_files() + file_list = changed_files(args) shutil.copy('.flake8', os.path.join(temp, '.flake8')) print('=======================================================') @@ -182,7 +207,7 @@ def flake8(parser, args): print() print('Modified files:') for filename in file_list: - print(" %s" % filename.strip()) + print(' {0}'.format(filename.strip())) print('=======================================================') # filter files into a temporary directory with exemptions added. @@ -202,20 +227,20 @@ def flake8(parser, args): else: # print results relative to current working directory def cwd_relative(path): - return '%s: [' % os.path.relpath( - os.path.join(spack.prefix, path.group(1)), os.getcwd()) + return '{0}: ['.format(os.path.relpath( + os.path.join(spack.prefix, path.group(1)), os.getcwd())) for line in output.split('\n'): print(re.sub(r'^(.*): \[', cwd_relative, line)) if flake8.returncode != 0: - print("Flake8 found errors.") + print('Flake8 found errors.') sys.exit(1) else: - print("Flake8 checks were clean.") + print('Flake8 checks were clean.') finally: if args.keep_temp: - print("temporary files are in ", temp) + print('Temporary files are in: ', temp) else: shutil.rmtree(temp, ignore_errors=True) diff --git a/lib/spack/spack/test/cmd/flake8.py b/lib/spack/spack/test/cmd/flake8.py new file mode 100644 index 0000000000..bde8d199a7 --- /dev/null +++ b/lib/spack/spack/test/cmd/flake8.py @@ -0,0 +1,95 @@ +############################################################################## +# Copyright (c) 2013-2016, Lawrence Livermore National Security, LLC. +# Produced at the Lawrence Livermore National Laboratory. +# +# This file is part of Spack. +# Created by Todd Gamblin, tgamblin@llnl.gov, All rights reserved. +# LLNL-CODE-647188 +# +# For details, see https://github.com/llnl/spack +# Please also see the LICENSE file for our notice and the LGPL. +# +# This program is free software; you can redistribute it and/or modify +# it under the terms of the GNU Lesser General Public License (as +# published by the Free Software Foundation) version 2.1, February 1999. +# +# This program is distributed in the hope that it will be useful, but +# WITHOUT ANY WARRANTY; without even the IMPLIED WARRANTY OF +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the terms and +# conditions of the GNU Lesser General Public License for more details. +# +# You should have received a copy of the GNU Lesser General Public +# License along with this program; if not, write to the Free Software +# Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA +############################################################################## +import argparse +import os +import pytest +import sys + +from llnl.util.filesystem import FileFilter + +import spack +from spack.cmd.flake8 import * +from spack.repository import Repo + + +@pytest.fixture(scope='module') +def parser(): + """Returns the parser for the ``flake8`` command""" + parser = argparse.ArgumentParser() + setup_parser(parser) + return parser + + +@pytest.fixture(scope='module') +def flake8_package(): + """Flake8 only checks files that have been modified. + This fixture makes a small change to the ``flake8`` + mock package, yields the filename, then undoes the + change on cleanup. + """ + repo = Repo(spack.mock_packages_path) + filename = repo.filename_for_package_name('flake8') + package = FileFilter(filename) + + # Make the change + package.filter('unmodified', 'modified') + + yield filename + + # Undo the change + package.filter('modified', 'unmodified') + + +def test_changed_files(parser, flake8_package): + args = parser.parse_args() + + # changed_files returns file paths relative to the root + # directory of Spack. Convert to absolute file paths. + files = changed_files(args) + files = [os.path.join(spack.spack_root, path) for path in files] + + # There will likely be other files that have changed + # when these tests are run + assert flake8_package in files + + +# As of flake8 3.0.0, Python 2.6 and 3.3 are no longer supported +# http://flake8.pycqa.org/en/latest/release-notes/3.0.0.html +@pytest.mark.skipif( + sys.version_info[:2] <= (2, 6) or + (3, 0) <= sys.version_info[:2] <= (3, 3), + reason='flake8 no longer supports Python 2.6 or 3.3 and older') +def test_flake8(parser, flake8_package): + # Only test the flake8_package that we modified + # Otherwise, the unit tests would fail every time + # the flake8 tests fail + args = parser.parse_args([flake8_package]) + + flake8(parser, args) + + # Get even more coverage + args = parser.parse_args(['--output', '--root-relative', flake8_package]) + + flake8(parser, args) -- cgit v1.2.3-60-g2f50