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
This commit is contained in:
Todd Gamblin
parent
b5d2c30d26
commit
24a4d81097
@ -11,8 +11,10 @@
|
||||
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 <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 cwd_relative(path):
|
||||
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
|
||||
@tool("isort")
|
||||
def run_isort(isort_cmd, file_list, args):
|
||||
check_fix_args = () if args.fix else ("--check", "--diff")
|
||||
|
||||
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")
|
||||
|
||||
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 prefix_relative(path):
|
||||
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
|
||||
|
||||
@ -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
|
||||
|
||||
@ -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
|
||||
|
||||
Loading…
Reference in New Issue
Block a user