Use YAML 1.2 by default for reformatting

Fixes: #3790

Author: ssbarnea

.config/requirements-lock.txt

@@ -14,7 +14,7 @@ cffi==1.16.0
 charset-normalizer==3.3.1
 click==8.1.7
 cryptography==41.0.5
-filelock==3.12.4
+filelock==3.13.0
 idna==3.4
 importlib-resources==5.0.7
 jinja2==3.1.2
@@ -34,7 +34,7 @@ referencing==0.30.2
 requests==2.31.0
 rich==13.6.0
 rpds-py==0.10.6
-ruamel-yaml==0.18.2
+ruamel-yaml==0.18.3
 subprocess-tee==0.4.1
 tomli==2.0.1
 typing-extensions==4.8.0

.config/requirements.txt

@@ -29,7 +29,7 @@ defusedxml==0.7.1
 dill==0.3.7
 exceptiongroup==1.1.3
 execnet==2.0.2
-filelock==3.12.4
+filelock==3.13.0
 ghp-import==2.1.0
 griffe==0.36.4
 htmlmin2==0.1.13
@@ -91,7 +91,7 @@ regex==2023.8.8
 requests==2.31.0
 rich==13.6.0
 rpds-py==0.10.6
-ruamel-yaml==0.18.2
+ruamel-yaml==0.18.3
 six==1.16.0
 soupsieve==2.5
 subprocess-tee==0.4.1

.github/workflows/tox.yml

@@ -70,7 +70,7 @@ jobs:
     env:
       # Number of expected test passes, safety measure for accidental skip of
       # tests. Update value if you add/remove tests.
-      PYTEST_REQPASS: 834
+      PYTEST_REQPASS: 845
     steps:
       - uses: actions/checkout@v4
         with:

.pre-commit-config.yaml

@@ -131,7 +131,7 @@ repos:
         types: [file, yaml]
         entry: yamllint --strict
   - repo: https://github.com/astral-sh/ruff-pre-commit
-    rev: "v0.1.2"
+    rev: "v0.1.3"
     hooks:
       - id: ruff
         args: [--fix, --exit-non-zero-on-fix]

src/ansiblelint/transformer.py

@@ -8,7 +8,11 @@
 
 from ansiblelint.file_utils import Lintable
 from ansiblelint.rules import AnsibleLintRule, TransformMixin
-from ansiblelint.yaml_utils import FormattedYAML, get_path_to_play, get_path_to_task
+from ansiblelint.yaml_utils import (
+    FormattedYAML,
+    get_path_to_play,
+    get_path_to_task,
+)
 
 if TYPE_CHECKING:
     from ansiblelint.config import Options

src/ansiblelint/yaml_utils.py

@@ -44,6 +44,7 @@
 
 _logger = logging.getLogger(__name__)
 
+
 YAMLLINT_CONFIG = """
 extends: default
 rules:
@@ -750,13 +751,14 @@ def write_version_directive(self, version_text: Any) -> None:
 class FormattedYAML(YAML):
     """A YAML loader/dumper that handles ansible content better by default."""
 
-    def __init__(
+    def __init__(  # pylint: disable=too-many-arguments
         self,
         *,
         typ: str | None = None,
         pure: bool = False,
         output: Any = None,
         plug_ins: list[str] | None = None,
+        version: VersionType | None = None,
     ):
         """Return a configured ``ruamel.yaml.YAML`` instance.
 
@@ -817,8 +819,11 @@ def __init__(
                 - name: Task
         """
         # Default to reading/dumping YAML 1.1 (ruamel.yaml defaults to 1.2)
-        self._yaml_version_default: tuple[int, int] = (1, 1)
-        self._yaml_version: str | tuple[int, int] = self._yaml_version_default
+        if version:
+            if isinstance(version, str):
+                version = tuple(version.split("."))
+            self._yaml_version_default: VersionType = version
+            self._yaml_version: VersionType = self._yaml_version_default
 
         super().__init__(typ=typ, pure=pure, output=output, plug_ins=plug_ins)
 
@@ -874,6 +879,9 @@ def __init__(
         # (see https://stackoverflow.com/a/44314840/1134951)
         # self.Representer.add_representer(
 
+        if version:
+            self.version = version
+
     @staticmethod
     def _defaults_from_yamllint_config() -> dict[str, bool | int | str]:
         """Extract FormattedYAML-relevant settings from yamllint config if possible."""
@@ -920,16 +928,18 @@ def _defaults_from_yamllint_config() -> dict[str, bool | int | str]:
 
         return cast(dict[str, Union[bool, int, str]], config)
 
-    @property  # type: ignore[override]
-    def version(self) -> str | tuple[int, int]:
+    @property
+    def version(self) -> VersionType | None:
         """Return the YAML version used to parse or dump.
 
         Ansible uses PyYAML which only supports YAML 1.1. ruamel.yaml defaults to 1.2.
         So, we have to make sure we dump yaml files using YAML 1.1.
         We can relax the version requirement once ansible uses a version of PyYAML
         that includes this PR: https://github.com/yaml/pyyaml/pull/555
         """
-        return self._yaml_version
+        if hasattr(self, "_yaml_version"):
+            return self._yaml_version
+        return None
 
     @version.setter
     def version(self, value: str | tuple[int, int] | None) -> None:
@@ -939,7 +949,11 @@ def version(self, value: str | tuple[int, int] | None) -> None:
         So, if a file does not include the directive, it sets this to None.
         But, None effectively resets the parsing version to YAML 1.2 (ruamel's default).
         """
-        self._yaml_version = value if value is not None else self._yaml_version_default
+        if value is not None:
+            self._yaml_version = value
+        elif hasattr(self, "_yaml_version_default"):
+            self._yaml_version = self._yaml_version_default
+        # We do nothing if the object did not had a previous default version defined
 
     def load(self, stream: Path | StreamTextType) -> Any:
         """Load YAML content from a string while avoiding known ruamel.yaml issues."""
@@ -958,6 +972,9 @@ def load(self, stream: Path | StreamTextType) -> Any:
         except ParserError:
             data = None
             _logger.error("Invalid yaml, verify the file contents and try again.")
+        except Exception as exc:
+            breakpoint()
+            raise exc
         if preamble_comment is not None and isinstance(
             data,
             (CommentedMap, CommentedSeq),
@@ -977,7 +994,11 @@ def dumps(self, data: Any) -> str:
                 stream.write(preamble_comment)
             self.dump(data, stream)
             text = stream.getvalue()
-        return self._post_process_yaml(text)
+        strip_version_directive = hasattr(self, "_yaml_version_default")
+        return self._post_process_yaml(
+            text,
+            strip_version_directive=strip_version_directive,
+        )
 
     def _prevent_wrapping_flow_style(self, data: Any) -> None:
         if not isinstance(data, (CommentedMap, CommentedSeq)):
@@ -1065,7 +1086,7 @@ def _pre_process_yaml(self, text: str) -> tuple[str, str | None]:
         return text, "".join(preamble_comments) or None
 
     @staticmethod
-    def _post_process_yaml(text: str) -> str:
+    def _post_process_yaml(text: str, *, strip_version_directive: bool = False) -> str:
         """Handle known issues with ruamel.yaml dumping.
 
         Make sure there's only one newline at the end of the file.
@@ -1077,6 +1098,10 @@ def _post_process_yaml(text: str) -> str:
 
         Make sure null list items don't end in a space.
         """
+        # remove YAML directive
+        if strip_version_directive and text.startswith("%YAML"):
+            text = text.split("\n", 1)[1]
+
         text = text.rstrip("\n") + "\n"
 
         lines = text.splitlines(keepends=True)

test/test_yaml_utils.py

@@ -15,6 +15,7 @@
 
 if TYPE_CHECKING:
     from ruamel.yaml.comments import CommentedMap, CommentedSeq
+    from ruamel.yaml.compat import VersionType
     from ruamel.yaml.emitter import Emitter
 
 fixtures_dir = Path(__file__).parent / "fixtures"
@@ -219,6 +220,39 @@ def fixture_yaml_formatting_fixtures(fixture_filename: str) -> tuple[str, str, s
     return before_content, prettier_content, formatted_content
 
 
+@pytest.mark.parametrize(
+    ("before", "after", "version"),
+    (
+        pytest.param("---\nfoo: bar\n", "---\nfoo: bar\n", None, id="1"),
+        # verify that 'on' is not translated to bool (1.2 behavior)
+        pytest.param("---\nfoo: on\n", "---\nfoo: on\n", None, id="2"),
+        # When version is manually mentioned by us, we expect to output without version directive
+        pytest.param("---\nfoo: on\n", "---\nfoo: on\n", (1, 2), id="3"),
+        pytest.param("---\nfoo: on\n", "---\nfoo: true\n", (1, 1), id="4"),
+        pytest.param("%YAML 1.1\n---\nfoo: on\n", "---\nfoo: true\n", (1, 1), id="5"),
+        # verify that in-line directive takes precedence but dumping strips if we mention a specific version
+        pytest.param("%YAML 1.1\n---\nfoo: on\n", "---\nfoo: true\n", (1, 2), id="6"),
+        # verify that version directive are kept if present
+        pytest.param("%YAML 1.1\n---\nfoo: on\n", "---\nfoo: true\n", None, id="7"),
+        pytest.param(
+            "%YAML 1.2\n---\nfoo: on\n",
+            "%YAML 1.2\n---\nfoo: on\n",
+            None,
+            id="8",
+        ),
+        pytest.param("---\nfoo: YES\n", "---\nfoo: true\n", (1, 1), id="9"),
+        pytest.param("---\nfoo: YES\n", "---\nfoo: YES\n", (1, 2), id="10"),
+        pytest.param("---\nfoo: YES\n", "---\nfoo: YES\n", None, id="11"),
+    ),
+)
+def test_fmt(before: str, after: str, version: VersionType) -> None:
+    """Tests behavior of formatter in regards to different YAML versions, specified or not."""
+    yaml = ansiblelint.yaml_utils.FormattedYAML(version=version)
+    data = yaml.load(before)
+    result = yaml.dumps(data)
+    assert result == after
+
+
 @pytest.mark.parametrize(
     "fixture_filename",
     (
@@ -239,7 +273,7 @@ def test_formatted_yaml_loader_dumper(
     assert before_content != prettier_content
     assert before_content != after_content
 
-    yaml = ansiblelint.yaml_utils.FormattedYAML()
+    yaml = ansiblelint.yaml_utils.FormattedYAML(version=(1,1))
 
     data_before = yaml.load(before_content)
     dump_from_before = yaml.dumps(data_before)
@@ -263,6 +297,9 @@ def test_formatted_yaml_loader_dumper(
     # Running our files through yamllint, after we reformatted them,
     # should not yield any problems.
     config = ansiblelint.yaml_utils.load_yamllint_config()
+    # We disable truthy rule as since we switched to default YAML 1.2 loading
+    # on ruamel.yaml, we expect to not auto-repair these on dumping.
+    config.rules["truthy"] = False
     assert not list(run_yamllint(after_content, config))