zhangyiss/spack
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

black: fix style check package and flake8 formatting for black

Browse Source 
Black will automatically fix a lot of the exceptions we previously allowed for
directives, so we don't need them in our custom `flake8_formatter` anymore.

- [x] remove `E501` (long line) exceptions for directives from `flake8_formatter`,
      as they won't help us now.
- [x] Refine exceptions for long URLs in the `flake8_formatter`.
- [x] Adjust the mock `flake8-package` to exhibit the exceptions we still allow.
- [x] Update style tests for new `flake8-package`.
- [x] Blacken style test.
This commit is contained in:
Todd Gamblin 2022-07-24 17:00:10 -07:00
parent 156af2a60a
commit 549ba1ed32
6 changed files with 87 additions and 114 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

6
lib/spack/spack/test/cmd/extensions.py
View File

@ -39,13 +39,11 @@ def check_output(ni, na):
installed = extensions("-s", "installed", "python") installed = extensions("-s", "installed", "python")
activated = extensions("-s", "activated", "python") activated = extensions("-s", "activated", "python")
assert "==> python@2.7.11" in output assert "==> python@2.7.11" in output
assert "==> 3 extensions" in output assert "==> 2 extensions" in output
assert "flake8" in output
assert "py-extension1" in output assert "py-extension1" in output
assert "py-extension2" in output assert "py-extension2" in output
assert "==> 3 extensions" in packages assert "==> 2 extensions" in packages
assert "flake8" in packages
assert "py-extension1" in packages assert "py-extension1" in packages
assert "py-extension2" in packages assert "py-extension2" in packages
assert "installed" not in packages assert "installed" not in packages

1
lib/spack/spack/test/cmd/resource.py
View File

@ -18,7 +18,6 @@
'c45c1564f70def3fc1a6e22139f62cb21cd190cc3a7dbe6f4120fa59ce33dcb8', 'c45c1564f70def3fc1a6e22139f62cb21cd190cc3a7dbe6f4120fa59ce33dcb8',
'24eceabef5fe8f575ff4b438313dc3e7b30f6a2d1c78841fbbe3b9293a589277', '24eceabef5fe8f575ff4b438313dc3e7b30f6a2d1c78841fbbe3b9293a589277',
'689b8f9b32cb1d2f9271d29ea3fca2e1de5df665e121fca14e1364b711450deb', '689b8f9b32cb1d2f9271d29ea3fca2e1de5df665e121fca14e1364b711450deb',
'ebe27f9930b99ebd8761ed2db3ea365142d0bafd78317efb4baadf62c7bf94d0',
'208fcfb50e5a965d5757d151b675ca4af4ce2dfd56401721b6168fae60ab798f', '208fcfb50e5a965d5757d151b675ca4af4ce2dfd56401721b6168fae60ab798f',
'bf07a7fbb825fc0aae7bf4a1177b2b31fcf8a3feeaf7092761e18c859ee52a9c', 'bf07a7fbb825fc0aae7bf4a1177b2b31fcf8a3feeaf7092761e18c859ee52a9c',
'7d865e959b2466918c9863afca942d0fb89d7c9ac0c99bafc3749504ded97730', '7d865e959b2466918c9863afca942d0fb89d7c9ac0c99bafc3749504ded97730',

68
lib/spack/spack/test/cmd/style.py
View File

@ -19,29 +19,29 @@
from spack.util.executable import which from spack.util.executable import which
#: directory with sample style files #: directory with sample style files
style_data = os.path.join(spack.paths.test_path, 'data', 'style') style_data = os.path.join(spack.paths.test_path, "data", "style")
style = spack.main.SpackCommand("style") style = spack.main.SpackCommand("style")
def has_develop_branch(): def has_develop_branch():
git = which('git') git = which("git")
if not git: if not git:
return False return False
git("show-ref", "--verify", "--quiet", git("show-ref", "--verify", "--quiet", "refs/heads/develop", fail_on_error=False)
"refs/heads/develop", fail_on_error=False)
return git.returncode == 0 return git.returncode == 0
# spack style requires git to run -- skip the tests if it's not there # spack style requires git to run -- skip the tests if it's not there
pytestmark = pytest.mark.skipif(not has_develop_branch(), pytestmark = pytest.mark.skipif(
reason='requires git with develop branch') not has_develop_branch(), reason="requires git with develop branch"
)
# The style tools have requirements to use newer Python versions. We simplify by # The style tools have requirements to use newer Python versions. We simplify by
# requiring Python 3.6 or higher to run spack style. # requiring Python 3.6 or higher to run spack style.
skip_old_python = pytest.mark.skipif( skip_old_python = pytest.mark.skipif(
sys.version_info < (3, 6), reason='requires Python 3.6 or higher' sys.version_info < (3, 6), reason="requires Python 3.6 or higher"
) )
@ -74,11 +74,13 @@ def flake8_package_with_errors(scope="function"):
try: try:
shutil.copy(filename, tmp) shutil.copy(filename, tmp)
package = FileFilter(filename) package = FileFilter(filename)
package.filter("state = 'unmodified'", "state = 'modified'", string=True)
# this is a black error (quote style and spacing before/after operator)
package.filter('state = "unmodified"', "state = 'modified'", string=True)
# this is an isort error (orderign) and a flake8 error (unused import)
package.filter( package.filter(
"from spack.package import *", "from spack.package import *", "from spack.package import *\nimport os", string=True
"from spack.package import *\nimport os",
string=True
) )
yield filename yield filename
finally: finally:
@ -88,10 +90,7 @@ def flake8_package_with_errors(scope="function"):
def test_changed_files(flake8_package): def test_changed_files(flake8_package):
# changed_files returns file paths relative to the root # changed_files returns file paths relative to the root
# directory of Spack. Convert to absolute file paths. # directory of Spack. Convert to absolute file paths.
files = [ files = [os.path.join(spack.paths.prefix, os.path.normpath(path)) for path in changed_files()]
os.path.join(spack.paths.prefix, os.path.normpath(path))
for path in changed_files()
]
# There will likely be other files that have changed # There will likely be other files that have changed
# when these tests are run # when these tests are run
@ -108,11 +107,11 @@ def test_changed_files_from_git_rev_base(tmpdir, capfd):
git("config", "user.email", "test@user.com") git("config", "user.email", "test@user.com")
git("commit", "--allow-empty", "-m", "initial commit") git("commit", "--allow-empty", "-m", "initial commit")
tmpdir.ensure('bin/spack') tmpdir.ensure("bin/spack")
assert changed_files(base="HEAD") == ['bin/spack'] assert changed_files(base="HEAD") == ["bin/spack"]
assert changed_files(base="main") == ['bin/spack'] assert changed_files(base="main") == ["bin/spack"]
git("add", 'bin/spack') git("add", "bin/spack")
git("commit", "-m", "v1") git("commit", "-m", "v1")
assert changed_files(base="HEAD") == [] assert changed_files(base="HEAD") == []
assert changed_files(base="HEAD~") == ["bin/spack"] assert changed_files(base="HEAD~") == ["bin/spack"]
@ -138,10 +137,12 @@ def test_changed_no_base(tmpdir, capfd):
def test_changed_files_all_files(flake8_package): def test_changed_files_all_files(flake8_package):
# it's hard to guarantee "all files", so do some sanity checks. # it's hard to guarantee "all files", so do some sanity checks.
files = set([ files = set(
os.path.join(spack.paths.prefix, os.path.normpath(path)) [
for path in changed_files(all_files=True) os.path.join(spack.paths.prefix, os.path.normpath(path))
]) for path in changed_files(all_files=True)
]
)
# spack has a lot of files -- check that we're in the right ballpark # spack has a lot of files -- check that we're in the right ballpark
assert len(files) > 6000 assert len(files) > 6000
@ -180,12 +181,8 @@ def test_bad_root(tmpdir):
def test_style_is_package(tmpdir): def test_style_is_package(tmpdir):
"""Ensure the is_package() function works.""" """Ensure the is_package() function works."""
assert spack.cmd.style.is_package( assert spack.cmd.style.is_package("var/spack/repos/builtin/packages/hdf5/package.py")
"var/spack/repos/builtin/packages/hdf5/package.py" assert spack.cmd.style.is_package("var/spack/repos/builtin/packages/zlib/package.py")
)
assert spack.cmd.style.is_package(
"var/spack/repos/builtin/packages/zlib/package.py"
)
assert not spack.cmd.style.is_package("lib/spack/spack/spec.py") assert not spack.cmd.style.is_package("lib/spack/spack/spec.py")
assert not spack.cmd.style.is_package("lib/spack/external/pytest.py") assert not spack.cmd.style.is_package("lib/spack/external/pytest.py")
@ -241,10 +238,10 @@ def test_fix_style(external_style_root):
assert not filecmp.cmp(broken_py, fixed_py) assert not filecmp.cmp(broken_py, fixed_py)
output = style( output = style(
"--root", str(tmpdir), "--root",
"--no-mypy", # mypy doesn't fix, so skip it str(tmpdir),
"--no-mypy", # mypy doesn't fix, so skip it
"--no-flake8", # flake8 doesn't fix, so skip it "--no-flake8", # flake8 doesn't fix, so skip it
"--black",
"--fix", "--fix",
) )
print(output) print(output)
@ -262,10 +259,7 @@ def test_external_root(external_style_root):
# make sure tools are finding issues with external root, # make sure tools are finding issues with external root,
# not the real one. # not the real one.
output = style( output = style("--root-relative", "--root", str(tmpdir), fail_on_error=False)
"--root-relative", "--black", "--root", str(tmpdir),
fail_on_error=False
)
# make sure it failed # make sure it failed
assert style.returncode != 0 assert style.returncode != 0
@ -330,7 +324,7 @@ def test_style_with_errors(flake8_package_with_errors):
@pytest.mark.skipif(not which("flake8"), reason="flake8 is not installed.") @pytest.mark.skipif(not which("flake8"), reason="flake8 is not installed.")
@pytest.mark.skipif(not which("black"), reason="black is not installed.") @pytest.mark.skipif(not which("black"), reason="black is not installed.")
def test_style_with_black(flake8_package_with_errors): def test_style_with_black(flake8_package_with_errors):
output = style("--black", flake8_package_with_errors, fail_on_error=False) output = style(flake8_package_with_errors, fail_on_error=False)
assert "black found errors" in output assert "black found errors" in output
assert style.returncode != 0 assert style.returncode != 0
assert "spack style found errors" in output assert "spack style found errors" in output

39
share/spack/qa/flake8_formatter.py
View File

@ -21,24 +21,6 @@
r"^from spack.package import \*$", r"^from spack.package import \*$",
r"^from spack.package_defs import \*$", r"^from spack.package_defs import \*$",
], ],
# Exempt lines with urls and descriptions from overlong line errors.
"E501": [
r"^\s*homepage\s*=",
r"^\s*url\s*=",
r"^\s*git\s*=",
r"^\s*svn\s*=",
r"^\s*hg\s*=",
r"^\s*pypi\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. # Exempt '@when' decorated functions from redefinition errors.
"F811": [ "F811": [
r"^\s*@when\(.*\)", r"^\s*@when\(.*\)",
@ -47,7 +29,7 @@
# exemptions applied to all files. # exemptions applied to all files.
r".py$": { r".py$": {
"E501": [ "E501": [
r"(https?|ftp|file)\:", # URLs r"(ssh|https?|ftp|file)\:", # URLs
r'([\'"])[0-9a-fA-F]{32,}\1', # long hex checksums r'([\'"])[0-9a-fA-F]{32,}\1', # long hex checksums
] ]
}, },
@ -58,10 +40,7 @@
pattern_exemptions = dict( pattern_exemptions = dict(
( (
re.compile(file_pattern), re.compile(file_pattern),
dict( dict((code, [re.compile(p) for p in patterns]) for code, patterns in error_dict.items()),
(code, [re.compile(p) for p in patterns])
for code, patterns in error_dict.items()
),
) )
for file_pattern, error_dict in pattern_exemptions.items() for file_pattern, error_dict in pattern_exemptions.items()
) )
@ -105,9 +84,7 @@ def handle(self, error): # type: (Violation) -> None
# get list of patterns for this error code # get list of patterns for this error code
pats = self.spack_errors.get(error.code, None) pats = self.spack_errors.get(error.code, None)
# if any pattern matches, skip line # if any pattern matches, skip line
if pats is not None and any( if pats is not None and any((pat.search(error.physical_line) for pat in pats)):
(pat.search(error.physical_line) for pat in pats)
):
return return
# Special F811 handling # Special F811 handling
@ -117,16 +94,10 @@ def handle(self, error): # type: (Violation) -> None
# https://gitlab.com/pycqa/flake8/issues/583 # https://gitlab.com/pycqa/flake8/issues/583
# we can only determine if F811 should be ignored given the previous # we can only determine if F811 should be ignored given the previous
# line, so get the previous line and check it # line, so get the previous line and check it
if ( if self.spack_errors.get("F811", False) and error.code == "F811" and error.line_number > 1:
self.spack_errors.get("F811", False)
and error.code == "F811"
and error.line_number > 1
):
if self.file_lines is None: if self.file_lines is None:
if self.filename in {"stdin", "-", "(none)", None}: if self.filename in {"stdin", "-", "(none)", None}:
self.file_lines = pycodestyle.stdin_get_value().splitlines( self.file_lines = pycodestyle.stdin_get_value().splitlines(True)
True
)
else: else:
self.file_lines = pycodestyle.readlines(self.filename) self.file_lines = pycodestyle.readlines(self.filename)
for pat in self.spack_errors["F811"]: for pat in self.spack_errors["F811"]:

1
var/spack/repos/builtin.mock/packages/flake8/hyper-specific-patch-that-fixes-some-random-bug-that-probably-only-affects-one-user.patch
View File

@ -1 +0,0 @@
Fake patch

86
var/spack/repos/builtin.mock/packages/flake8/package.py
View File

@ -7,62 +7,74 @@
class Flake8(Package): class Flake8(Package):
"""Package containing as many PEP 8 violations as possible. """Package containing as many acceptable ``PEP8`` violations as possible.
All of these violations are exceptions that we allow in
package.py files.""" All of these violations are exceptions that we allow in ``package.py`` files, and
Spack is more lenient than ``flake8`` is for things like URLs and long SHA sums.
See ``share/spack/qa/flake8_formatter.py`` for specifics of how we handle ``flake8``
exemptions.
"""
# Used to tell whether or not the package has been modified # Used to tell whether or not the package has been modified
state = 'unmodified' state = "unmodified"
# Make sure pre-existing noqa is not interfered with # Make sure pre-existing noqa is not interfered with
blatant_violation = 'line-that-has-absolutely-no-execuse-for-being-over-79-characters' # noqa # note that black can sometimes fix shorter assignment statements by sticking them in
blatant_violation = 'line-that-has-absolutely-no-execuse-for-being-over-79-characters' # noqa: E501 # parens and adding line breaks, e.g.:
#
# foo = (
# "too-long-string"
# )
#
# but the one below can't even be fixed that way -- you have to add noqa, or break
# it up inside parens yourself.
blatant_violation = "line-that-has-absolutely-no-execuse-for-being-over-99-characters-and-that-black-cannot-fix-with-parens" # noqa: E501
# Keywords exempt from line-length checks # All URL strings are exempt from line-length checks.
homepage = '#####################################################################' #
url = '#####################################################################' # flake8 normally would complain about these, but the fix it wants (a multi-line
git = '#####################################################################' # string) is ugbly, and we're more lenient since there are many places where Spack
svn = '#####################################################################' # wants URLs in strings.
hg = '#####################################################################' hg = "https://example.com/this-is-a-really-long-url/that-goes-over-99-characters/that-flake8-will-not-ignore-by-default"
list_url = '#####################################################################' list_url = "https://example.com/this-is-a-really-long-url/that-goes-over-99-characters/that-flake8-will-not-ignore-by-default"
git = "ssh://example.com/this-is-a-really-long-url/that-goes-over-99-characters/that-flake8-will-not-ignore-by-default"
# URL strings exempt from line-length checks # directives with URLs are exempt as well
# http://######################################################################## version(
# https://####################################################################### "1.0",
# ftp://######################################################################### url="https://example.com/this-is-a-really-long-url/that-goes-over-99-characters/that-flake8-will-not-ignore-by-default",
# file://######################################################################## )
# Directives exempt from line-length checks #
version('2.0', '0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef') # Also test URL comments (though flake8 will ignore these by default anyway)
version('1.0', '0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef') #
# http://example.com/this-is-a-really-long-url/that-goes-over-99-characters/that-flake8-will-ignore-by-default
# https://example.com/this-is-a-really-long-url/that-goes-over-99-characters/that-flake8-will-ignore-by-default
# ftp://example.com/this-is-a-really-long-url/that-goes-over-99-characters/that-flake8-will-ignore-by-default
# ssh://example.com/this-is-a-really-long-url/that-goes-over-99-characters/that-flake8-will-ignore-by-default
# file://example.com/this-is-a-really-long-url/that-goes-over-99-characters/that-flake8-will-ignore-by-default
variant('super-awesome-feature', default=True, description='Enable super awesome feature') # Strings and comments with really long checksums require no noqa annotation.
variant('somewhat-awesome-feature', default=False, description='Enable somewhat awesome feature') sha512sum = "abcdef1234567890abcdef1234567890abcdef1234567890abcdef1234567890abcdef1234567890abcdef1234567890abcdef1234567890abcdef1234567890"
# the sha512sum is "abcdef1234567890abcdef1234567890abcdef1234567890abcdef1234567890abcdef1234567890abcdef1234567890abcdef1234567890abcdef1234567890"
provides('somevirt', when='@2.0+super-awesome-feature+somewhat-awesome-feature')
extends('python', ignore='bin/(why|does|every|package|that|depends|on|numpy|need|to|copy|f2py3?)')
depends_on('boost+atomic+chrono+date_time~debug+filesystem~graph~icu+iostreams+locale+log+math~mpi+multithreaded+program_options~python+random+regex+serialization+shared+signals~singlethreaded+system~taggedlayout+test+thread+timer+wave')
conflicts('+super-awesome-feature', when='%intel@16:17+somewhat-awesome-feature')
resource(name='Deez-Nuts', destination='White-House', placement='President', when='@2020', url='www.elect-deez-nuts.com')
patch('hyper-specific-patch-that-fixes-some-random-bug-that-probably-only-affects-one-user.patch', when='%gcc@3.2.2:3.2.3')
def install(self, spec, prefix): def install(self, spec, prefix):
# Make sure lines with '# noqa' work as expected. Don't just # Make sure lines with '# noqa' work as expected. Don't just
# remove them entirely. This will mess up the indentation of # remove them entirely. This will mess up the indentation of
# the following lines. # the following lines.
if 'really-long-if-statement' != 'that-goes-over-the-line-length-limit-and-requires-noqa': # noqa if (
"really-long-if-statement"
!= "this-string-is-so-long-that-it-is-over-the-line-limit-and-black-will-not-split-it-so-it-requires-noqa" # noqa: E501
):
pass pass
# sanity_check_prefix requires something in the install directory # sanity_check_prefix requires something in the install directory
mkdirp(prefix.bin) mkdirp(prefix.bin)
# '@when' decorated functions are exempt from redefinition errors # '@when' decorated functions are exempt from redefinition errors
@when('@2.0') @when("@2.0")
def install(self, spec, prefix): def install(self, spec, prefix):
# sanity_check_prefix requires something in the install directory # sanity_check_prefix requires something in the install directory
mkdirp(prefix.bin) mkdirp(prefix.bin)