From 7757ebc0bc5b46cb3cfa41e5bebd4754978fd3b0 Mon Sep 17 00:00:00 2001 From: Todd Gamblin Date: Mon, 23 Oct 2017 14:57:46 +0200 Subject: [PATCH] flake8: no wildcards in core; only `import *` from spack in packages There are now separate flake8 configs for core vs. packages: - core has a smaller set of flake8 exceptions - packages allow `from spack import *` and module globals - Allows core to take advantage of static checking for undefined names - Allows packages to keep using Spack tricks like `from spack import *` and dependencies setting globals for dependents. --- .flake8 | 13 ++---- .flake8_packages | 22 +++++++++ lib/spack/spack/cmd/flake8.py | 71 +++++++++++++++++++++--------- lib/spack/spack/test/cmd/flake8.py | 1 + 4 files changed, 78 insertions(+), 29 deletions(-) create mode 100644 .flake8_packages diff --git a/.flake8 b/.flake8 index e697d5ea04..49199b35c8 100644 --- a/.flake8 +++ b/.flake8 @@ -1,8 +1,8 @@ # -*- conf -*- -# flake8 settings for Spack. +# flake8 settings for Spack core files. # -# Below we describe which flake8 checks Spack ignores and what the -# rationale is. +# These exceptions ar for Spack core files. We're slightly more lenient +# with packages. See .flake8_packages for that. # # Let people line things up nicely: # - E129: visually indented line with same indent as next logical line @@ -13,14 +13,9 @@ # Let people use terse Python features: # - E731: lambda expressions # -# Spack allows wildcard imports: -# - F403: disable wildcard import -# # These are required to get the package.py files to test clean: -# - F405: `name` may be undefined, or undefined from star imports: `module` -# - F821: undefined name `name` (needed for cmake, configure, etc.) # - F999: syntax error in doctest # [flake8] -ignore = E129,E221,E241,E272,E731,F403,F405,F821,F999 +ignore = E129,E221,E241,E272,E731,F999 max-line-length = 79 diff --git a/.flake8_packages b/.flake8_packages new file mode 100644 index 0000000000..9fcc3b86d4 --- /dev/null +++ b/.flake8_packages @@ -0,0 +1,22 @@ +# -*- conf -*- +# flake8 settings for Spack package files. +# +# This should include all the same exceptions that we use for core files. +# +# In Spack packages, we also allow the single `from spack import *` +# wildcard import and dependencies can set globals for their +# dependents. So we add exceptions for checks related to undefined names. +# +# Note that we also add *per-line* exemptions for certain patters in the +# `spack flake8` command. This is where F403 for `from spack import *` +# is added (beause we *only* allow that wildcard). +# +# See .flake8 for regular exceptions. +# +# Redefinition exceptions: +# - F405: `name` may be undefined, or undefined from star imports: `module` +# - F821: undefined name `name` (needed for cmake, configure, etc.) +# +[flake8] +ignore = E129,E221,E241,E272,E731,F999,F405,F821 +max-line-length = 79 diff --git a/lib/spack/spack/cmd/flake8.py b/lib/spack/spack/cmd/flake8.py index 8f9e311734..01e642235d 100644 --- a/lib/spack/spack/cmd/flake8.py +++ b/lib/spack/spack/cmd/flake8.py @@ -42,18 +42,34 @@ level = "long" -"""List of directories to exclude from checks.""" +def is_package(f): + """Whether flake8 should consider a file as a core file or a package. + + We run flake8 with different exceptions for the core and for + packages, since we allow `from spack import *` and poking globals + into packages. + """ + return f.startswith('var/spack/repos/') or 'docs/tutorial/examples' in f + + +#: List of directories to exclude from checks. exclude_directories = [spack.external_path] -""" -This is a dict that maps: - filename pattern -> - a flake8 exemption code -> - list of patterns, for which matching lines should have codes applied. -""" -exemptions = { + +#: This is a dict that maps: +#: filename pattern -> +#: flake8 exemption code -> +#: list of patterns, for which matching lines should have codes applied. +#: +#: For each file, if the filename pattern matches, we'll add per-line +#: exemptions if any patterns in the sub-dict match. +pattern_exemptions = { # exemptions applied only to package.py files. r'package.py$': { + # Allow 'from spack import *' in packages, but no other wildcards + 'F403': [ + r'^from spack import \*$' + ], # Exempt lines with urls and descriptions from overlong line errors. 'E501': [ r'^\s*homepage\s*=', @@ -87,10 +103,11 @@ } # compile all regular expressions. -exemptions = dict((re.compile(file_pattern), - dict((code, [re.compile(p) for p in patterns]) - for code, patterns in error_dict.items())) - for file_pattern, error_dict in exemptions.items()) +pattern_exemptions = dict( + (re.compile(file_pattern), + dict((code, [re.compile(p) for p in patterns]) + for code, patterns in error_dict.items())) + for file_pattern, error_dict in pattern_exemptions.items()) def changed_files(args): @@ -135,7 +152,7 @@ def changed_files(args): return sorted(changed) -def add_exemptions(line, codes): +def add_pattern_exemptions(line, codes): """Add a flake8 exemption to a line.""" if line.startswith('#'): return line @@ -163,7 +180,7 @@ def add_exemptions(line, codes): def filter_file(source, dest, output=False): - """Filter a single file through all the patterns in exemptions.""" + """Filter a single file through all the patterns in pattern_exemptions.""" with open(source) as infile: parent = os.path.dirname(dest) mkdirp(parent) @@ -171,7 +188,9 @@ def filter_file(source, dest, output=False): with open(dest, 'w') as outfile: for line in infile: line_errors = [] - for file_pattern, errors in exemptions.items(): + + # pattern exemptions + for file_pattern, errors in pattern_exemptions.items(): if not file_pattern.search(source): continue @@ -182,7 +201,7 @@ def filter_file(source, dest, output=False): break if line_errors: - line = add_exemptions(line, line_errors) + line = add_pattern_exemptions(line, line_errors) outfile.write(line) if output: @@ -226,7 +245,6 @@ def prefix_relative(path): with working_dir(spack.prefix): if not file_list: file_list = changed_files(args) - shutil.copy('.flake8', os.path.join(temp, '.flake8')) print('=======================================================') print('flake8: running flake8 code checks on spack.') @@ -242,10 +260,23 @@ def prefix_relative(path): dest_path = os.path.join(temp, filename) filter_file(src_path, dest_path, args.output) - # run flake8 on the temporary tree. + # run flake8 on the temporary tree, once for core, once for pkgs + package_file_list = [f for f in file_list if is_package(f)] + file_list = [f for f in file_list if not is_package(f)] + with working_dir(temp): - output = flake8('--format', 'pylint', *file_list, - fail_on_error=False, output=str) + output = '' + if file_list: + output += flake8( + '--format', 'pylint', + '--config=%s' % os.path.join(spack.prefix, '.flake8'), + *file_list, fail_on_error=False, output=str) + if package_file_list: + output += flake8( + '--format', 'pylint', + '--config=%s' % os.path.join(spack.prefix, + '.flake8_packages'), + *package_file_list, fail_on_error=False, output=str) if args.root_relative: # print results relative to repo root. diff --git a/lib/spack/spack/test/cmd/flake8.py b/lib/spack/spack/test/cmd/flake8.py index 8556a0a036..b6433e95c9 100644 --- a/lib/spack/spack/test/cmd/flake8.py +++ b/lib/spack/spack/test/cmd/flake8.py @@ -34,6 +34,7 @@ from spack.repository import Repo + @pytest.fixture(scope='module') def parser(): """Returns the parser for the ``flake8`` command"""