style: simplify arguments with --tool TOOL and --skip TOOL
`spack style` tests were annoyingly brittle because we could not easily be
specific about which tools to run (we had to use `--no-black`, `--no-isort`,
`--no-flake8`, and `--no-mypy`). We should be able to specify what to run OR
what to skip.
Now you can run, e.g.:
spack style --tool black,flake8
or:
spack style --skip black,isort
- [x] Remove `--no-black`, `--no-isort`, `--no-flake8`, and `--no-mypy` args.
- [x] Add `--tool TOOL` argument.
- [x] Add `--skip TOOL` argument.
- [x] Allow either `--tool black --tool flake8` or `--tool black,flake8` syntax.
This commit is contained in:
Todd Gamblin
parent
76b190a624
commit
143f3f830c
3 changed files with 69 additions and 59 deletions
|
|
@ -46,7 +46,8 @@ def grouper(iterable, n, fillvalue=None):
|
|||
|
||||
#: 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)
|
||||
#: The list maps an executable name to a spack spec needed to install it.
|
||||
#: The list maps an executable name to a method to ensure the tool is
|
||||
#: bootstrapped or present in the environment.
|
||||
tool_order = [
|
||||
("isort", spack.bootstrap.ensure_isort_in_path_or_raise),
|
||||
("mypy", spack.bootstrap.ensure_mypy_in_path_or_raise),
|
||||
|
|
@ -54,6 +55,9 @@ def grouper(iterable, n, fillvalue=None):
|
|||
("flake8", spack.bootstrap.ensure_flake8_in_path_or_raise),
|
||||
]
|
||||
|
||||
#: list of just the tool names -- for argparse
|
||||
tool_names = [k for k, _ in tool_order]
|
||||
|
||||
#: tools we run in spack style
|
||||
tools = {}
|
||||
|
||||
|
|
@ -179,36 +183,28 @@ def setup_parser(subparser):
|
|||
default=False,
|
||||
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: run isort if available)",
|
||||
)
|
||||
subparser.add_argument(
|
||||
"--no-flake8",
|
||||
dest="flake8",
|
||||
action="store_false",
|
||||
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: run mypy if available)",
|
||||
)
|
||||
subparser.add_argument(
|
||||
"--no-black",
|
||||
dest="black",
|
||||
action="store_false",
|
||||
help="run black if available (default: run black if available)",
|
||||
)
|
||||
subparser.add_argument(
|
||||
"--root",
|
||||
action="store",
|
||||
default=None,
|
||||
help="style check a different spack instance",
|
||||
)
|
||||
|
||||
tool_group = subparser.add_mutually_exclusive_group()
|
||||
tool_group.add_argument(
|
||||
"-t",
|
||||
"--tool",
|
||||
action="append",
|
||||
help="specify which tools to run (default: %s)" % ",".join(tool_names),
|
||||
)
|
||||
tool_group.add_argument(
|
||||
"-s",
|
||||
"--skip",
|
||||
metavar="TOOL",
|
||||
action="append",
|
||||
help="specify tools to skip (choose from %s)" % ",".join(tool_names),
|
||||
)
|
||||
|
||||
subparser.add_argument("files", nargs=argparse.REMAINDER, help="specific files to check")
|
||||
|
||||
|
||||
|
|
@ -233,8 +229,8 @@ def translate(match):
|
|||
print(line)
|
||||
|
||||
|
||||
def print_style_header(file_list, args):
|
||||
tools = [tool for tool, _ in tool_order if getattr(args, tool)]
|
||||
def print_style_header(file_list, args, selected):
|
||||
tools = [tool for tool in tool_names if tool in selected]
|
||||
tty.msg("Running style checks on spack", "selected: " + ", ".join(tools))
|
||||
|
||||
# translate modified paths to cwd_relative if needed
|
||||
|
|
@ -394,6 +390,15 @@ def run_black(black_cmd, file_list, args):
|
|||
return returncode
|
||||
|
||||
|
||||
def validate_toolset(arg_value):
|
||||
"""Validate --tool and --skip arguments (sets of optionally comma-separated tools)."""
|
||||
tools = set(",".join(arg_value).split(",")) # allow args like 'isort,flake8'
|
||||
for tool in tools:
|
||||
if tool not in tool_names:
|
||||
tty.die("Invaild tool: '%s'" % tool, "Choose from: %s" % ", ".join(tool_names))
|
||||
return tools
|
||||
|
||||
|
||||
def style(parser, args):
|
||||
# ensure python version is new enough
|
||||
if sys.version_info < (3, 6):
|
||||
|
|
@ -421,26 +426,33 @@ def prefix_relative(path):
|
|||
|
||||
file_list = [prefix_relative(p) for p in file_list]
|
||||
|
||||
# process --tool and --skip arguments
|
||||
selected = set(tool_names)
|
||||
if args.tool is not None:
|
||||
selected = validate_toolset(args.tool)
|
||||
if args.skip is not None:
|
||||
selected -= validate_toolset(args.skip)
|
||||
|
||||
if not selected:
|
||||
tty.msg("Nothing to run.")
|
||||
return
|
||||
|
||||
return_code = 0
|
||||
with working_dir(args.root):
|
||||
if not file_list:
|
||||
file_list = changed_files(args.base, args.untracked, args.all)
|
||||
print_style_header(file_list, args)
|
||||
|
||||
print_style_header(file_list, args, selected)
|
||||
|
||||
tools_to_run = [(tool, fn) for tool, fn in tool_order if tool in selected]
|
||||
commands = {}
|
||||
with spack.bootstrap.ensure_bootstrap_configuration():
|
||||
for tool_name, bootstrap_fn in tool_order:
|
||||
# Skip the tool if it was not requested
|
||||
if not getattr(args, tool_name):
|
||||
continue
|
||||
|
||||
# bootstrap everything first to get commands
|
||||
for tool_name, bootstrap_fn in tools_to_run:
|
||||
commands[tool_name] = bootstrap_fn()
|
||||
|
||||
for tool_name, bootstrap_fn in tool_order:
|
||||
# Skip the tool if it was not requested
|
||||
if not getattr(args, tool_name):
|
||||
continue
|
||||
|
||||
# run tools once bootstrapping is done
|
||||
for tool_name, bootstrap_fn in tools_to_run:
|
||||
run_function, required = tools[tool_name]
|
||||
print_tool_header(tool_name)
|
||||
return_code |= run_function(commands[tool_name], file_list, args)
|
||||
|
|
|
|||
|
|
@ -224,8 +224,8 @@ def external_style_root(flake8_package_with_errors, tmpdir):
|
|||
|
||||
|
||||
@skip_old_python
|
||||
@pytest.mark.skipif(not which("isort"), reason="isort is not installed.")
|
||||
@pytest.mark.skipif(not which("black"), reason="black is not installed.")
|
||||
@pytest.mark.skipif(not which("flake8"), reason="flake8 is not installed.")
|
||||
def test_fix_style(external_style_root):
|
||||
"""Make sure spack style --fix works."""
|
||||
tmpdir, py_file = external_style_root
|
||||
|
|
@ -237,14 +237,9 @@ def test_fix_style(external_style_root):
|
|||
shutil.copy(broken_dummy, broken_py)
|
||||
assert not filecmp.cmp(broken_py, fixed_py)
|
||||
|
||||
output = style(
|
||||
"--root",
|
||||
str(tmpdir),
|
||||
"--no-mypy", # mypy doesn't fix, so skip it
|
||||
"--no-flake8", # flake8 doesn't fix, so skip it
|
||||
"--fix",
|
||||
)
|
||||
print(output)
|
||||
# black and isort are the tools that actually fix things
|
||||
style("--root", str(tmpdir), "--tool", "isort,black", "--fix")
|
||||
|
||||
assert filecmp.cmp(broken_py, fixed_py)
|
||||
|
||||
|
||||
|
|
@ -288,24 +283,19 @@ def test_style(flake8_package, tmpdir):
|
|||
with tmpdir.as_cwd():
|
||||
relative = os.path.relpath(flake8_package)
|
||||
|
||||
# no args
|
||||
output = style(fail_on_error=False)
|
||||
assert relative in output
|
||||
assert "spack style checks were clean" in output
|
||||
|
||||
# one specific arg
|
||||
output = style(flake8_package, fail_on_error=False)
|
||||
output = style("--tool", "flake8", flake8_package, fail_on_error=False)
|
||||
assert relative in output
|
||||
assert "spack style checks were clean" in output
|
||||
|
||||
# specific file that isn't changed
|
||||
output = style(__file__, fail_on_error=False)
|
||||
output = style("--tool", "flake8", __file__, fail_on_error=False)
|
||||
assert relative not in output
|
||||
assert __file__ in output
|
||||
assert "spack style checks were clean" in output
|
||||
|
||||
# root-relative paths
|
||||
output = style("--root-relative", flake8_package)
|
||||
output = style("--tool", "flake8", "--root-relative", flake8_package)
|
||||
assert root_relative in output
|
||||
assert "spack style checks were clean" in output
|
||||
|
||||
|
|
@ -314,17 +304,25 @@ def test_style(flake8_package, tmpdir):
|
|||
@pytest.mark.skipif(not which("flake8"), reason="flake8 is not installed.")
|
||||
def test_style_with_errors(flake8_package_with_errors):
|
||||
root_relative = os.path.relpath(flake8_package_with_errors, spack.paths.prefix)
|
||||
output = style("--root-relative", flake8_package_with_errors, fail_on_error=False)
|
||||
output = style(
|
||||
"--tool", "flake8", "--root-relative", flake8_package_with_errors, fail_on_error=False
|
||||
)
|
||||
assert root_relative in output
|
||||
assert style.returncode != 0
|
||||
assert "spack style found errors" in output
|
||||
|
||||
|
||||
@skip_old_python
|
||||
@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("flake8"), reason="flake8 is not installed.")
|
||||
def test_style_with_black(flake8_package_with_errors):
|
||||
output = style(flake8_package_with_errors, fail_on_error=False)
|
||||
output = style("--tool", "black,flake8", 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
|
||||
|
||||
|
||||
@skip_old_python
|
||||
def test_skip_tools():
|
||||
output = style("--skip", "isort,mypy,black,flake8")
|
||||
assert "Nothing to run" in output
|
||||
|
|
|
|||
|
|
@ -1697,7 +1697,7 @@ _spack_stage() {
|
|||
_spack_style() {
|
||||
if $list_options
|
||||
then
|
||||
SPACK_COMPREPLY="-h --help -b --base -a --all -r --root-relative -U --no-untracked -f --fix --no-isort --no-flake8 --no-mypy --no-black --root"
|
||||
SPACK_COMPREPLY="-h --help -b --base -a --all -r --root-relative -U --no-untracked -f --fix --root -t --tool -s --skip"
|
||||
else
|
||||
SPACK_COMPREPLY=""
|
||||
fi
|
||||
|
|
|
|||
Loading…
Reference in a new issue