From 24a4d81097c2a930b34d55e25400d14b74ec4112 Mon Sep 17 00:00:00 2001 From: Todd Gamblin Date: Sun, 4 Jul 2021 01:53:02 -0700 Subject: style: clean up and restructure `spack style` command This consolidates code across tools in `spack style` so that each `run_` 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 --- lib/spack/spack/cmd/style.py | 170 ++++++++++++++++++++----------------------- 1 file changed, 77 insertions(+), 93 deletions(-) (limited to 'lib') 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) @@ -114,12 +132,6 @@ def setup_parser(subparser): action="store_true", 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", @@ -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 :: 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 -- cgit v1.2.3-60-g2f50