style: clean up and restructure `spack style` command

This consolidates code across tools in `spack style` so that each
`run_<tool>` function can be called indirecty through a dictionary
of handlers, and os that checks like finding the executable for the
tool can be shared across commands.

- [x] rework `spack style` to use decorators to register tools
- [x] define tool order in one place in `spack style`
- [x] fix python 2/3 issues to Get `isort` checks working
- [x] make isort error regex more robust across versions
- [x] remove unused output option
- [x] change vestigial `TRAVIS_BRANCH` to `GITHUB_BASE_REF`
- [x] update completion

3 files changed, 80 insertions, 96 deletions

diff --git a/lib/spack/spack/cmd/style.py b/lib/spack/spack/cmd/style.py
index 23fc3e37ae..dec8cbad4e 100644
--- a/lib/spack/spack/cmd/style.py
+++ b/lib/spack/spack/cmd/style.py
@@ -11,8 +11,10 @@ import re
 import sys
 
 import llnl.util.tty as tty
-import spack.paths
+import llnl.util.tty.color as color
 from llnl.util.filesystem import working_dir
+
+import spack.paths
 from spack.util.executable import which
 
 if sys.version_info < (3, 0):
@@ -39,8 +41,12 @@ def grouper(iterable, n, fillvalue=None):
 #: List of directories to exclude from checks.
 exclude_directories = [spack.paths.external_path]
 
-#: max line length we're enforcing (note: this duplicates what's in .flake8)
-max_line_length = 79
+#: order in which tools should be run. flake8 is last so that it can
+#: double-check the results of other tools (if, e.g., --fix was provided)
+tool_order = ["isort", "mypy", "black", "flake8"]
+
+#: tools we run in spack style
+tools = {}
 
 
 def is_package(f):
@@ -53,13 +59,25 @@ def is_package(f):
     return f.startswith("var/spack/repos/") or "docs/tutorial/examples" in f
 
 
+#: decorator for adding tools to the list
+class tool(object):
+    def __init__(self, name, required=False):
+        self.name = name
+        self.required = required
+
+    def __call__(self, fun):
+        tools[self.name] = (fun, self.required)
+        return fun
+
+
 def changed_files(base=None, untracked=True, all_files=False):
     """Get list of changed files in the Spack repository."""
 
     git = which("git", required=True)
 
+    # GITHUB_BASE_REF is set to the base branch for pull request actions
     if base is None:
-        base = os.environ.get("TRAVIS_BRANCH", "develop")
+        base = os.environ.get("GITHUB_BASE_REF", "develop")
 
     range = "{0}...".format(base)
 
@@ -115,12 +133,6 @@ def setup_parser(subparser):
         help="check all files, not just changed files",
     )
     subparser.add_argument(
-        "-o",
-        "--output",
-        action="store_true",
-        help="send filtered files to stdout as well as temp files",
-    )
-    subparser.add_argument(
         "-r",
         "--root-relative",
         action="store_true",
@@ -140,31 +152,31 @@ def setup_parser(subparser):
         "--fix",
         action="store_true",
         default=False,
-        help="If the tool supports automatically editing file contents, do that.",
+        help="format automatically if possible (e.g., with isort, black)",
     )
     subparser.add_argument(
         "--no-isort",
         dest="isort",
         action="store_false",
-        help="Do not run isort, default is run isort if available",
+        help="do not run isort (default: run isort if available)",
     )
     subparser.add_argument(
         "--no-flake8",
         dest="flake8",
         action="store_false",
-        help="Do not run flake8, default is run flake8",
+        help="do not run flake8 (default: run flake8 or fail)",
     )
     subparser.add_argument(
         "--no-mypy",
         dest="mypy",
         action="store_false",
-        help="Do not run mypy, default is run mypy if available",
+        help="do not run mypy (default: run mypy if available)",
     )
     subparser.add_argument(
         "--black",
         dest="black",
         action="store_true",
-        help="Run black checks, default is skip",
+        help="run black if available (default: skip black)",
     )
     subparser.add_argument(
         "files", nargs=argparse.REMAINDER, help="specific files to check"
@@ -177,7 +189,8 @@ def rewrite_and_print_output(
     """rewrite ouput with <file>:<line>: format to respect path args"""
     if args.root_relative or re_obj is None:
         # print results relative to repo root.
-        print(output)
+        for line in output.split("\n"):
+            print("  " + output)
     else:
         # print results relative to current working directory
         def cwd_relative(path):
@@ -191,34 +204,32 @@ def rewrite_and_print_output(
         for line in output.split("\n"):
             if not line:
                 continue
-            print(re_obj.sub(cwd_relative, line))
+            print("  " + re_obj.sub(cwd_relative, line))
 
 
 def print_style_header(file_list, args):
-    tty.msg("style: running code checks on spack.")
-    tools = []
-    if args.flake8:
-        tools.append("flake8")
-    if args.mypy:
-        tools.append("mypy")
-    if args.black:
-        tools.append("black")
-    tty.msg("style: tools selected: " + ", ".join(tools))
+    tools = [tool for tool in tool_order if getattr(args, tool)]
+    tty.msg("Running style checks on spack:", "selected: " + ", ".join(tools))
     tty.msg("Modified files:", *[filename.strip() for filename in file_list])
     sys.stdout.flush()
 
 
 def print_tool_header(tool):
     sys.stdout.flush()
-    tty.msg("style: running %s checks on spack." % tool)
+    tty.msg("Running %s checks" % tool)
     sys.stdout.flush()
 
 
-def run_flake8(file_list, args):
-    returncode = 0
-    print_tool_header("flake8")
-    flake8_cmd = which("flake8", required=True)
+def print_tool_result(tool, returncode):
+    if returncode == 0:
+        color.cprint("  @g{%s checks were clean}" % tool)
+    else:
+        color.cprint("  @r{%s found errors}" % tool)
 
+
+@tool("flake8", required=True)
+def run_flake8(flake8_cmd, file_list, args):
+    returncode = 0
     output = ""
     # run in chunks of 100 at a time to avoid line length limit
     # filename parameter in config *does not work* for this reliably
@@ -237,20 +248,12 @@ def run_flake8(file_list, args):
 
         rewrite_and_print_output(output, args)
 
-    if returncode == 0:
-        tty.msg("Flake8 style checks were clean")
-    else:
-        tty.error("Flake8 style checks found errors")
+    print_tool_result("flake8", returncode)
     return returncode
 
 
-def run_mypy(file_list, args):
-    mypy_cmd = which("mypy")
-    if mypy_cmd is None:
-        tty.error("style: mypy is not available in path, skipping")
-        return 1
-
-    print_tool_header("mypy")
+@tool("mypy")
+def run_mypy(mypy_cmd, file_list, args):
     mpy_args = ["--package", "spack", "--package", "llnl", "--show-error-codes"]
     # not yet, need other updates to enable this
     # if any([is_package(f) for f in file_list]):
@@ -261,49 +264,30 @@ def run_mypy(file_list, args):
 
     rewrite_and_print_output(output, args)
 
-    if returncode == 0:
-        tty.msg("mypy checks were clean")
-    else:
-        tty.error("mypy checks found errors")
+    print_tool_result("mypy", returncode)
     return returncode
 
 
-def run_isort(file_list, args):
-    isort_cmd = which("isort")
-    if isort_cmd is None:
-        tty.error("style: isort is not available in path, skipping")
-        return 1
-
-    print_tool_header("isort")
-    # TODO: isort apparently decides to avoid writing colored output at all unless the
-    # output is a tty, even when --color is provided.
-    check_fix_args = () if args.fix else ("--check", "--diff", "--color")
+@tool("isort")
+def run_isort(isort_cmd, file_list, args):
+    check_fix_args = () if args.fix else ("--check", "--diff")
 
-    pat = re.compile("ERROR: (.*) Imports are incorrectly sorted and/or formatted")
-    replacement = "ERROR: {0} Imports are incorrectly sorted and/or formatted"
+    pat = re.compile("ERROR: (.*) Imports are incorrectly sorted")
+    replacement = "ERROR: {0} Imports are incorrectly sorted"
     returncode = 0
     for chunk in grouper(file_list, 100):
-        # TODO: py2 does not support multiple * unpacking expressions in a single call.
-        packed_args = check_fix_args + chunk
+        packed_args = check_fix_args + tuple(chunk)
         output = isort_cmd(*packed_args, fail_on_error=False, output=str, error=str)
         returncode |= isort_cmd.returncode
 
         rewrite_and_print_output(output, args, pat, replacement)
 
-    if returncode == 0:
-        tty.msg("isort style checks were clean")
-    else:
-        tty.error("isort checks found errors")
+    print_tool_result("isort", returncode)
     return returncode
 
 
-def run_black(file_list, args):
-    black_cmd = which("black")
-    if black_cmd is None:
-        tty.error("style: black is not available in path, skipping")
-        return 1
-
-    print_tool_header("black")
+@tool("black")
+def run_black(black_cmd, file_list, args):
     check_fix_args = () if args.fix else ("--check", "--diff", "--color")
 
     pat = re.compile("would reformat +(.*)")
@@ -313,17 +297,13 @@ def run_black(file_list, args):
     # run in chunks of 100 at a time to avoid line length limit
     # filename parameter in config *does not work* for this reliably
     for chunk in grouper(file_list, 100):
-        # TODO: py2 does not support multiple * unpacking expressions in a single call.
-        packed_args = check_fix_args + chunk
+        packed_args = check_fix_args + tuple(chunk)
         output = black_cmd(*packed_args, fail_on_error=False, output=str, error=str)
         returncode |= black_cmd.returncode
 
         rewrite_and_print_output(output, args, pat, replacement)
 
-    if returncode == 0:
-        tty.msg("black style checks were clean")
-    else:
-        tty.error("black checks found errors")
+    print_tool_result("black", returncode)
     return returncode
 
 
@@ -343,20 +323,24 @@ def style(parser, args):
         if not file_list:
             file_list = changed_files(args.base, args.untracked, args.all)
         print_style_header(file_list, args)
-        if args.isort:
-            # We run isort before flake8, assuming that #23947 with flake8-import-order
-            # is also in, to allow flake8 to double-check the result of executing isort,
-            # if --fix was provided.
-            returncode = run_isort(file_list, args)
-        if args.flake8:
-            returncode |= run_flake8(file_list, args)
-        if args.mypy:
-            returncode |= run_mypy(file_list, args)
-        if args.black:
-            returncode |= run_black(file_list, args)
-
-    if returncode != 0:
-        print("spack style found errors.")
-        sys.exit(1)
+
+        # run tools in order defined in tool_order
+        returncode = 0
+        for tool_name in tool_order:
+            if getattr(args, tool_name):
+                run_function, required = tools[tool_name]
+                print_tool_header(tool_name)
+
+                cmd = which(tool_name, required=required)
+                if not cmd:
+                    color.cprint("  @y{%s not in PATH, skipped}" % tool_name)
+                    continue
+
+                returncode |= run_function(cmd, file_list, args)
+
+    if returncode == 0:
+        tty.msg(color.colorize("@*{spack style checks were clean}"))
     else:
-        print("spack style checks were clean.")
+        tty.error(color.colorize("@*{spack style found errors}"))
+
+    return returncode
diff --git a/share/spack/qa/run-style-tests b/share/spack/qa/run-style-tests
index 23d756a3bc..4eb503dae2 100755
--- a/share/spack/qa/run-style-tests
+++ b/share/spack/qa/run-style-tests
@@ -18,7 +18,7 @@
 check_dependencies flake8 mypy
 
 # verify that the code style is correct
-spack style
+spack style --root-relative
 
 # verify that the license headers are present
 spack license verify
diff --git a/share/spack/spack-completion.bash b/share/spack/spack-completion.bash
index 08a4c999c9..cd5a01c42d 100755
--- a/share/spack/spack-completion.bash
+++ b/share/spack/spack-completion.bash
@@ -978,7 +978,7 @@ _spack_find() {
 _spack_flake8() {
     if $list_options
     then
-        SPACK_COMPREPLY="-h --help -b --base -a --all -o --output -r --root-relative -U --no-untracked --no-flake8 --no-mypy --black"
+        SPACK_COMPREPLY="-h --help -b --base -a --all -r --root-relative -U --no-untracked -f --fix --no-isort --no-flake8 --no-mypy --black"
     else
         SPACK_COMPREPLY=""
     fi
@@ -1584,7 +1584,7 @@ _spack_stage() {
 _spack_style() {
     if $list_options
     then
-        SPACK_COMPREPLY="-h --help -b --base -a --all -o --output -r --root-relative -U --no-untracked --no-flake8 --no-mypy --black"
+        SPACK_COMPREPLY="-h --help -b --base -a --all -r --root-relative -U --no-untracked -f --fix --no-isort --no-flake8 --no-mypy --black"
     else
         SPACK_COMPREPLY=""
     fi