Skip to content

Commit

Permalink
Rename --write option to -fix
Browse files Browse the repository at this point in the history
This change will make ansible-lint auto-fixing feature in line with
other tools that can perform similar transformations (ruff,
black, ...).

For compatibility, we automatically convert old option into new one
and display a warning.
  • Loading branch information
ssbarnea committed Sep 19, 2023
1 parent 66378f6 commit 939be03
Show file tree
Hide file tree
Showing 6 changed files with 36 additions and 29 deletions.
2 changes: 1 addition & 1 deletion .ansible-lint
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,7 @@ warn_list:
# - yaml[document-start] # you can also use sub-rule matches

# Some rules can transform files to fix (or make it easier to fix) identified
# errors. `ansible-lint --write` will reformat YAML files and run these transforms.
# errors. `ansible-lint --fix` will reformat YAML files and run these transforms.
# By default it will run all transforms (effectively `write_list: ["all"]`).
# You can disable running transforms by setting `write_list: ["none"]`.
# Or only enable a subset of rule transforms by listing rules/tags here.
Expand Down
2 changes: 1 addition & 1 deletion src/ansiblelint/__main__.py
Original file line number Diff line number Diff line change
Expand Up @@ -274,7 +274,7 @@ def main(argv: list[str] | None = None) -> int:

if Version(ruamel_safe_version) > Version(ruamel_yaml_version_str):
_logger.warning(
"We detected use of `--write` feature with a buggy ruamel-yaml %s library instead of >=%s, upgrade it before reporting any bugs like dropped comments.",
"We detected use of `--fix` feature with a buggy ruamel-yaml %s library instead of >=%s, upgrade it before reporting any bugs like dropped comments.",
ruamel_yaml_version_str,
ruamel_safe_version,
)
Expand Down
2 changes: 1 addition & 1 deletion src/ansiblelint/app.py
Original file line number Diff line number Diff line change
Expand Up @@ -223,7 +223,7 @@ def report_outcome(

if self.options.write_list and "yaml" in self.options.skip_list:
_logger.warning(
"You specified '--write', but no files can be modified "
"You specified '--fix', but no files can be modified "
"because 'yaml' is in 'skip_list'.",
)

Expand Down
35 changes: 21 additions & 14 deletions src/ansiblelint/cli.py
Original file line number Diff line number Diff line change
Expand Up @@ -145,7 +145,7 @@ def __call__(


class WriteArgAction(argparse.Action):
"""Argparse action to handle the --write flag with optional args."""
"""Argparse action to handle the --fix flag with optional args."""

_default = "__default__"

Expand Down Expand Up @@ -174,8 +174,8 @@ def __init__( # pylint: disable=too-many-arguments,redefined-builtin
super().__init__(
option_strings=option_strings,
dest=dest,
nargs="?", # either 0 (--write) or 1 (--write=a,b,c) argument
const=self._default, # --write (no option) implicitly stores this
nargs="?", # either 0 (--fix) or 1 (--fix=a,b,c) argument
const=self._default, # --fix (no option) implicitly stores this
default=default,
type=type,
choices=choices,
Expand All @@ -194,8 +194,8 @@ def __call__(
lintables = getattr(namespace, "lintables", None)
if not lintables and isinstance(values, str):
# args are processed in order.
# If --write is after lintables, then that is not ambiguous.
# But if --write comes first, then it might actually be a lintable.
# If --fix is after lintables, then that is not ambiguous.
# But if --fix comes first, then it might actually be a lintable.
maybe_lintable = Path(values)
if maybe_lintable.exists():
namespace.lintables = [values]
Expand All @@ -216,14 +216,14 @@ def merge_write_list_config(
from_file: list[str],
from_cli: list[str],
) -> list[str]:
"""Combine the write_list from file config with --write CLI arg.
"""Combine the write_list from file config with --fix CLI arg.
Handles the implicit "all" when "__default__" is present and file config is empty.
"""
if not from_file or "none" in from_cli:
# --write is the same as --write=all
# --fix is the same as --fix=all
return ["all" if value == cls._default else value for value in from_cli]
# --write means use the config from the config file
# --fix means use the config from the config file
from_cli = [value for value in from_cli if value != cls._default]
return from_file + from_cli

Expand Down Expand Up @@ -338,7 +338,7 @@ def get_cli_parser() -> argparse.ArgumentParser:
help="Return non-zero exit code on warnings as well as errors",
)
parser.add_argument(
"--write",
"--fix",
dest="write_list",
# this is a tri-state argument that takes an optional comma separated list:
action=WriteArgAction,
Expand All @@ -347,12 +347,12 @@ def get_cli_parser() -> argparse.ArgumentParser:
"A rule transform can fix or simplify fixing issues identified by that rule). "
"You can limit the effective rule transforms (the 'write_list') by passing a "
"keywords 'all' or 'none' or a comma separated list of rule ids or rule tags. "
"YAML reformatting happens whenever '--write' or '--write=' is used. "
"'--write' and '--write=all' are equivalent: they allow all transforms to run. "
"YAML reformatting happens whenever '--fix' or '--fix=' is used. "
"'--fix' and '--fix=all' are equivalent: they allow all transforms to run. "
"The effective list of transforms comes from 'write_list' in the config file, "
"followed whatever '--write' args are provided on the commandline. "
"'--write=none' resets the list of transforms to allow reformatting YAML "
"without running any of the transforms (ie '--write=none,rule-id' will "
"followed whatever '--fix' args are provided on the commandline. "
"'--fix=none' resets the list of transforms to allow reformatting YAML "
"without running any of the transforms (ie '--fix=none,rule-id' will "
"ignore write_list in the config file and only run the rule-id transform).",
)
parser.add_argument(
Expand Down Expand Up @@ -557,6 +557,13 @@ def merge_config(file_config: dict[Any, Any], cli_config: Options) -> Options:
def get_config(arguments: list[str]) -> Options:
"""Extract the config based on given args."""
parser = get_cli_parser()
# translate deprecated options
for i, value in enumerate(arguments):
if arguments[i].startswith("--write"):
arguments[i] = value.replace("--write", "--fix")
_logger.warning(
"Replaced deprecated '--write' option with '--fix', change you call to avoid future regressions when we remove old option.",
)
options = Options(**vars(parser.parse_args(arguments)))

# docs is not document, being used for internal documentation building
Expand Down
2 changes: 1 addition & 1 deletion src/ansiblelint/rules/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -254,7 +254,7 @@ def matchyaml(self, file: Lintable) -> list[MatchError]:
class TransformMixin:
"""A mixin for AnsibleLintRule to enable transforming files.
If ansible-lint is started with the ``--write`` option, then the ``Transformer``
If ansible-lint is started with the ``--fix`` option, then the ``Transformer``
will call the ``transform()`` method for every MatchError identified if the rule
that identified it subclasses this ``TransformMixin``. Only the rule that identified
a MatchError can do transforms to fix that match.
Expand Down
22 changes: 11 additions & 11 deletions test/test_cli.py
Original file line number Diff line number Diff line change
Expand Up @@ -68,34 +68,34 @@ def test_ensure_config_are_equal(
@pytest.mark.parametrize(
("with_base", "args", "config"),
(
(True, ["--write"], "test/fixtures/config-with-write-all.yml"),
(True, ["--write=all"], "test/fixtures/config-with-write-all.yml"),
(True, ["--write", "all"], "test/fixtures/config-with-write-all.yml"),
(True, ["--write=none"], "test/fixtures/config-with-write-none.yml"),
(True, ["--write", "none"], "test/fixtures/config-with-write-none.yml"),
(True, ["--fix"], "test/fixtures/config-with-write-all.yml"),
(True, ["--fix=all"], "test/fixtures/config-with-write-all.yml"),
(True, ["--fix", "all"], "test/fixtures/config-with-write-all.yml"),
(True, ["--fix=none"], "test/fixtures/config-with-write-none.yml"),
(True, ["--fix", "none"], "test/fixtures/config-with-write-none.yml"),
(
True,
["--write=rule-tag,rule-id"],
["--fix=rule-tag,rule-id"],
"test/fixtures/config-with-write-subset.yml",
),
(
True,
["--write", "rule-tag,rule-id"],
["--fix", "rule-tag,rule-id"],
"test/fixtures/config-with-write-subset.yml",
),
(
True,
["--write", "rule-tag", "--write", "rule-id"],
["--fix", "rule-tag", "--fix", "rule-id"],
"test/fixtures/config-with-write-subset.yml",
),
(
False,
["--write", "examples/playbooks/example.yml"],
["--fix", "examples/playbooks/example.yml"],
"test/fixtures/config-with-write-all.yml",
),
(
False,
["--write", "examples/playbooks/example.yml", "non-existent.yml"],
["--fix", "examples/playbooks/example.yml", "non-existent.yml"],
"test/fixtures/config-with-write-all.yml",
),
),
Expand All @@ -106,7 +106,7 @@ def test_ensure_write_cli_does_not_consume_lintables(
args: list[str],
config: str,
) -> None:
"""Check equality of the CLI --write options to config files."""
"""Check equality of the CLI --fix options to config files."""
cli_parser = cli.get_cli_parser()

command = base_arguments + args if with_base else args
Expand Down

0 comments on commit 939be03

Please sign in to comment.