Use YAML 1.2 by default for reformatting

Fixes: #3790

Author: ssbarnea

.config/dictionary.txt

@@ -208,6 +208,7 @@ matchtasks
 matchvar
 matchyaml
 maxdepth
+maxsplit
 minversion
 mkdir
 mkdocs

.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]

pyproject.toml

@@ -233,7 +233,6 @@ python_files = [
 xfail_strict = true
 
 [tool.ruff]
-required-version = "0.1.2"
 ignore = [
   "D203", # incompatible with D211
   "D213", # incompatible with D212

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
@@ -92,7 +96,9 @@ def run(self) -> None:
                 # We need a fresh YAML() instance for each load because ruamel.yaml
                 # stores intermediate state during load which could affect loading
                 # any other files. (Based on suggestion from ruamel.yaml author)
-                yaml = FormattedYAML()
+                yaml = FormattedYAML(
+                    version=(1, 1) if file.is_owned_by_ansible() else None,
+                )
 
                 ruamel_data = yaml.load(data)
                 if not isinstance(ruamel_data, (CommentedMap, CommentedSeq)):

src/ansiblelint/yaml_utils.py

@@ -35,7 +35,7 @@
 if TYPE_CHECKING:
     # noinspection PyProtectedMember
     from ruamel.yaml.comments import LineCol
-    from ruamel.yaml.compat import StreamTextType
+    from ruamel.yaml.compat import StreamTextType, VersionType
     from ruamel.yaml.nodes import ScalarNode
     from ruamel.yaml.representer import RoundTripRepresenter
     from ruamel.yaml.tokens import CommentToken
@@ -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,12 @@ 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):
+                x, y = version.split(".", maxsplit=1)
+                version = (int(x), int(y))
+            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 +880,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 +929,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 +950,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."""
@@ -977,7 +992,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 +1084,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 +1096,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",
     (
@@ -229,7 +263,7 @@ def fixture_yaml_formatting_fixtures(fixture_filename: str) -> tuple[str, str, s
         pytest.param("fmt-5.yml", id="5"),
     ),
 )
-def test_formatted_yaml_loader_dumper(
+def test_formatted_yaml11_loader_dumper(
     yaml_formatting_fixtures: tuple[str, str, str],
     fixture_filename: str,  # noqa: ARG001
 ) -> None:
@@ -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))