black: fix style check package and flake8 formatting for black

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.

6 files changed, 90 insertions, 117 deletions

diff --git a/lib/spack/spack/test/cmd/extensions.py b/lib/spack/spack/test/cmd/extensions.py
index 14747d0439..88f073519c 100644
--- a/lib/spack/spack/test/cmd/extensions.py
+++ b/lib/spack/spack/test/cmd/extensions.py
@@ -39,13 +39,11 @@ def test_extensions(mock_packages, python_database, config, capsys):
             installed = extensions("-s", "installed", "python")
             activated = extensions("-s", "activated", "python")
         assert "==> python@2.7.11" in output
-        assert "==> 3 extensions" in output
-        assert "flake8" in output
+        assert "==> 2 extensions" in output
         assert "py-extension1" in output
         assert "py-extension2" in output
 
-        assert "==> 3 extensions" in packages
-        assert "flake8" in packages
+        assert "==> 2 extensions" in packages
         assert "py-extension1" in packages
         assert "py-extension2" in packages
         assert "installed" not in packages
diff --git a/lib/spack/spack/test/cmd/resource.py b/lib/spack/spack/test/cmd/resource.py
index 64a79fd601..c6dd537693 100644
--- a/lib/spack/spack/test/cmd/resource.py
+++ b/lib/spack/spack/test/cmd/resource.py
@@ -18,7 +18,6 @@ mock_hashes = [
     'c45c1564f70def3fc1a6e22139f62cb21cd190cc3a7dbe6f4120fa59ce33dcb8',
     '24eceabef5fe8f575ff4b438313dc3e7b30f6a2d1c78841fbbe3b9293a589277',
     '689b8f9b32cb1d2f9271d29ea3fca2e1de5df665e121fca14e1364b711450deb',
-    'ebe27f9930b99ebd8761ed2db3ea365142d0bafd78317efb4baadf62c7bf94d0',
     '208fcfb50e5a965d5757d151b675ca4af4ce2dfd56401721b6168fae60ab798f',
     'bf07a7fbb825fc0aae7bf4a1177b2b31fcf8a3feeaf7092761e18c859ee52a9c',
     '7d865e959b2466918c9863afca942d0fb89d7c9ac0c99bafc3749504ded97730',
diff --git a/lib/spack/spack/test/cmd/style.py b/lib/spack/spack/test/cmd/style.py
index 2b8330aa59..35e03b0387 100644
--- a/lib/spack/spack/test/cmd/style.py
+++ b/lib/spack/spack/test/cmd/style.py
@@ -19,29 +19,29 @@ from spack.cmd.style import changed_files
 from spack.util.executable import which
 
 #: 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")
 
 
 def has_develop_branch():
-    git = which('git')
+    git = which("git")
     if not git:
         return False
-    git("show-ref", "--verify", "--quiet",
-        "refs/heads/develop", fail_on_error=False)
+    git("show-ref", "--verify", "--quiet", "refs/heads/develop", fail_on_error=False)
     return git.returncode == 0
 
 
 # spack style requires git to run -- skip the tests if it's not there
-pytestmark = pytest.mark.skipif(not has_develop_branch(),
-                                reason='requires git with develop branch')
+pytestmark = pytest.mark.skipif(
+    not has_develop_branch(), reason="requires git with develop branch"
+)
 
 # The style tools have requirements to use newer Python versions.  We simplify by
 # requiring Python 3.6 or higher to run spack style.
 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:
         shutil.copy(filename, tmp)
         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(
-            "from spack.package import *",
-            "from spack.package import *\nimport os",
-            string=True
+            "from spack.package import *", "from spack.package import *\nimport os", string=True
         )
         yield filename
     finally:
@@ -88,10 +90,7 @@ def flake8_package_with_errors(scope="function"):
 def test_changed_files(flake8_package):
     # changed_files returns file paths relative to the root
     # directory of Spack. Convert to absolute file paths.
-    files = [
-        os.path.join(spack.paths.prefix, os.path.normpath(path))
-        for path in changed_files()
-    ]
+    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
     # 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("commit", "--allow-empty", "-m", "initial commit")
 
-        tmpdir.ensure('bin/spack')
-        assert changed_files(base="HEAD") == ['bin/spack']
-        assert changed_files(base="main") == ['bin/spack']
+        tmpdir.ensure("bin/spack")
+        assert changed_files(base="HEAD") == ["bin/spack"]
+        assert changed_files(base="main") == ["bin/spack"]
 
-        git("add", 'bin/spack')
+        git("add", "bin/spack")
         git("commit", "-m", "v1")
         assert changed_files(base="HEAD") == []
         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):
     # it's hard to guarantee "all files", so do some sanity checks.
-    files = set([
-        os.path.join(spack.paths.prefix, os.path.normpath(path))
-        for path in changed_files(all_files=True)
-    ])
+    files = set(
+        [
+            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
     assert len(files) > 6000
@@ -180,12 +181,8 @@ def test_bad_root(tmpdir):
 
 def test_style_is_package(tmpdir):
     """Ensure the is_package() function works."""
-    assert spack.cmd.style.is_package(
-        "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/hdf5/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/external/pytest.py")
 
@@ -241,10 +238,10 @@ def test_fix_style(external_style_root):
     assert not filecmp.cmp(broken_py, fixed_py)
 
     output = style(
-        "--root", str(tmpdir),
-        "--no-mypy",    # mypy doesn't fix, so skip it
+        "--root",
+        str(tmpdir),
+        "--no-mypy",  # mypy doesn't fix, so skip it
         "--no-flake8",  # flake8 doesn't fix, so skip it
-        "--black",
         "--fix",
     )
     print(output)
@@ -262,10 +259,7 @@ def test_external_root(external_style_root):
 
     # make sure tools are finding issues with external root,
     # not the real one.
-    output = style(
-        "--root-relative", "--black", "--root", str(tmpdir),
-        fail_on_error=False
-    )
+    output = style("--root-relative", "--root", str(tmpdir), fail_on_error=False)
 
     # make sure it failed
     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("black"), reason="black is not installed.")
 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 style.returncode != 0
     assert "spack style found errors" in output
diff --git a/share/spack/qa/flake8_formatter.py b/share/spack/qa/flake8_formatter.py
index 523ee7b4d0..93cb4cf3fb 100644
--- a/share/spack/qa/flake8_formatter.py
+++ b/share/spack/qa/flake8_formatter.py
@@ -21,24 +21,6 @@ pattern_exemptions = {
             r"^from spack.package 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.
         "F811": [
             r"^\s*@when\(.*\)",
@@ -47,7 +29,7 @@ pattern_exemptions = {
     # exemptions applied to all files.
     r".py$": {
         "E501": [
-            r"(https?|ftp|file)\:",  # URLs
+            r"(ssh|https?|ftp|file)\:",  # URLs
             r'([\'"])[0-9a-fA-F]{32,}\1',  # long hex checksums
         ]
     },
@@ -58,10 +40,7 @@ pattern_exemptions = {
 pattern_exemptions = dict(
     (
         re.compile(file_pattern),
-        dict(
-            (code, [re.compile(p) for p in patterns])
-            for code, patterns in error_dict.items()
-        ),
+        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()
 )
@@ -105,9 +84,7 @@ class SpackFormatter(Pylint):
         # get list of patterns for this error code
         pats = self.spack_errors.get(error.code, None)
         # if any pattern matches, skip line
-        if pats is not None and any(
-            (pat.search(error.physical_line) for pat in pats)
-        ):
+        if pats is not None and any((pat.search(error.physical_line) for pat in pats)):
             return
 
         # Special F811 handling
@@ -117,16 +94,10 @@ class SpackFormatter(Pylint):
         # https://gitlab.com/pycqa/flake8/issues/583
         # we can only determine if F811 should be ignored given the previous
         # line, so get the previous line and check it
-        if (
-            self.spack_errors.get("F811", False)
-            and error.code == "F811"
-            and error.line_number > 1
-        ):
+        if self.spack_errors.get("F811", False) and error.code == "F811" and error.line_number > 1:
             if self.file_lines is None:
                 if self.filename in {"stdin", "-", "(none)", None}:
-                    self.file_lines = pycodestyle.stdin_get_value().splitlines(
-                        True
-                    )
+                    self.file_lines = pycodestyle.stdin_get_value().splitlines(True)
                 else:
                     self.file_lines = pycodestyle.readlines(self.filename)
             for pat in self.spack_errors["F811"]:
diff --git a/var/spack/repos/builtin.mock/packages/flake8/hyper-specific-patch-that-fixes-some-random-bug-that-probably-only-affects-one-user.patch b/var/spack/repos/builtin.mock/packages/flake8/hyper-specific-patch-that-fixes-some-random-bug-that-probably-only-affects-one-user.patch
deleted file mode 100644
index b8c9065e4b..0000000000
--- a/var/spack/repos/builtin.mock/packages/flake8/hyper-specific-patch-that-fixes-some-random-bug-that-probably-only-affects-one-user.patch
+++ /dev/null
@@ -1 +0,0 @@
-Fake patch
diff --git a/var/spack/repos/builtin.mock/packages/flake8/package.py b/var/spack/repos/builtin.mock/packages/flake8/package.py
index ec9454cfec..d8ce33859f 100644
--- a/var/spack/repos/builtin.mock/packages/flake8/package.py
+++ b/var/spack/repos/builtin.mock/packages/flake8/package.py
@@ -7,62 +7,74 @@ from spack.package import *
 
 
 class Flake8(Package):
-    """Package containing as many PEP 8 violations as possible.
-    All of these violations are exceptions that we allow in
-    package.py files."""
+    """Package containing as many acceptable ``PEP8`` violations as possible.
 
-    # Used to tell whether or not the package has been modified
-    state = 'unmodified'
-
-    # Make sure pre-existing noqa is not interfered with
-    blatant_violation = 'line-that-has-absolutely-no-execuse-for-being-over-79-characters'  # noqa
-    blatant_violation = 'line-that-has-absolutely-no-execuse-for-being-over-79-characters'  # noqa: E501
-
-    # Keywords exempt from line-length checks
-    homepage = '#####################################################################'
-    url      = '#####################################################################'
-    git      = '#####################################################################'
-    svn      = '#####################################################################'
-    hg       = '#####################################################################'
-    list_url = '#####################################################################'
-
-    # URL strings exempt from line-length checks
-    # http://########################################################################
-    # https://#######################################################################
-    # ftp://#########################################################################
-    # file://########################################################################
-
-    # Directives exempt from line-length checks
-    version('2.0', '0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef')
-    version('1.0', '0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef')
+    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.
 
-    variant('super-awesome-feature',    default=True,  description='Enable super awesome feature')
-    variant('somewhat-awesome-feature', default=False, description='Enable somewhat awesome feature')
+    See ``share/spack/qa/flake8_formatter.py`` for specifics of how we handle ``flake8``
+    exemptions.
 
-    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')
+    # Used to tell whether or not the package has been modified
+    state = "unmodified"
 
-    patch('hyper-specific-patch-that-fixes-some-random-bug-that-probably-only-affects-one-user.patch', when='%gcc@3.2.2:3.2.3')
+    # Make sure pre-existing noqa is not interfered with
+    # note that black can sometimes fix shorter assignment statements by sticking them in
+    # 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
+
+    # All URL strings are exempt from line-length checks.
+    #
+    # flake8 normally would complain about these, but the fix it wants (a multi-line
+    # string) is ugbly, and we're more lenient since there are many places where Spack
+    # wants URLs in strings.
+    hg = "https://example.com/this-is-a-really-long-url/that-goes-over-99-characters/that-flake8-will-not-ignore-by-default"
+    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"
+
+    # directives with URLs are exempt as well
+    version(
+        "1.0",
+        url="https://example.com/this-is-a-really-long-url/that-goes-over-99-characters/that-flake8-will-not-ignore-by-default",
+    )
+
+    #
+    # Also test URL comments (though flake8 will ignore these by default anyway)
+    #
+    # 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
+
+    # Strings and comments with really long checksums require no noqa annotation.
+    sha512sum = "abcdef1234567890abcdef1234567890abcdef1234567890abcdef1234567890abcdef1234567890abcdef1234567890abcdef1234567890abcdef1234567890"
+    # the sha512sum is "abcdef1234567890abcdef1234567890abcdef1234567890abcdef1234567890abcdef1234567890abcdef1234567890abcdef1234567890abcdef1234567890"
 
     def install(self, spec, prefix):
         # Make sure lines with '# noqa' work as expected. Don't just
         # remove them entirely. This will mess up the indentation of
         # 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
 
         # sanity_check_prefix requires something in the install directory
         mkdirp(prefix.bin)
 
     # '@when' decorated functions are exempt from redefinition errors
-    @when('@2.0')
+    @when("@2.0")
     def install(self, spec, prefix):
         # sanity_check_prefix requires something in the install directory
         mkdirp(prefix.bin)