refactor: address review - improve error path, remove dead flow-seq code, rewrite docstring, introduce DiffMode enum

Author: nathanjmcdougall

.pre-commit-config.yaml

@@ -55,6 +55,10 @@ repos:
         entry: uv run --frozen cargo clippy --all-targets --no-default-features -- -D warnings
         language: system
         pass_filenames: false
+  - repo: https://github.com/jendrikseipp/vulture
+    rev: v2.16
+    hooks:
+      - id: vulture
   - repo: https://github.com/codespell-project/codespell
     rev: v2.4.2
     hooks:

pyproject.toml

@@ -32,6 +32,7 @@ dev = [
   "ruff>=0.15.12",
   "tomli>=2.4.1",
   "ty>=0.0.35",
+  "vulture>=2.16",
 ]
 test = [
   "coverage[toml]>=7.14.0",
@@ -75,3 +76,10 @@ report.exclude_also = [
   "raise NotImplementedError",
 ]
 report.omit = [ "*/pytest-of-*/*" ]
+
+[tool.vulture]
+paths = [ "src/yamltrip" ]
+ignore_names = [
+  "dump",     # Public API method on Document
+  "original", # Public API property on Editor
+]

src/yamltrip/document.py

@@ -547,13 +547,16 @@ def sync(self, *keys: KeyPart, value: Any) -> Document:
             return self._apply_patches([_core.Patch(route=route, operation=op)])
 
     def merge(self, *keys: KeyPart, value: Any) -> Document:
-        """Merge a value into the mapping at path without removing extra keys.
-
-        Recursively updates matching keys and adds new keys, but never
-        removes existing keys not present in *value*. Lists are replaced
-        entirely (not merged element-wise).
+        """Merge or replace the value at path, depending on node type.
+
+        If both the existing value and *value* are mappings, matching keys
+        are updated recursively and new keys are added, but existing keys not
+        present in *value* are preserved. Lists are replaced entirely (not
+        merged element-wise), and scalars are replaced. If the path does not
+        exist, it is created. If the existing node type differs from *value*,
+        the value at path is replaced.
         """
-        from yamltrip.sync import _compute_patches  # noqa: PLC0415
+        from yamltrip.sync import DiffMode, _compute_patches  # noqa: PLC0415
 
         normalized = _normalize_keys(keys) if keys else ()
 
@@ -567,7 +570,14 @@ def merge(self, *keys: KeyPart, value: Any) -> Document:
                     return self.upsert(*normalized, value=value)
                 except PatchError as e:
                     if "unexpected node" in str(e):
-                        msg = f"Value at {format_path(normalized)} is not a mapping"
+                        # Find deepest existing ancestor to report
+                        failing = normalized
+                        for i in range(len(normalized), 0, -1):
+                            sub = normalized[:i]
+                            if self._core_doc.query_exists(_make_route(sub)):
+                                failing = sub
+                                break
+                        msg = f"Value at {format_path(failing)} is not a mapping"
                         raise NodeTypeError(msg) from None
                     raise
 
@@ -580,26 +590,10 @@ def merge(self, *keys: KeyPart, value: Any) -> Document:
         if old_value == value:
             return self
 
-        # Pre-convert any flow sequences that will be modified.
-        doc: Document = self
-        flow_patches = _flow_seq_replacements(
-            self._core_doc, old_value, value, normalized
-        )
-        if flow_patches:
-            doc = doc._apply_patches(flow_patches)
-            old_value = doc._core_doc.parse_value(_make_route(normalized))
-
-        patches = _compute_patches(old_value, value, normalized, remove_extra=False)
+        patches = _compute_patches(old_value, value, normalized, mode=DiffMode.MERGE)
         if not patches:
-            return doc
-        try:
-            return doc._apply_patches(patches)
-        except PatchError as e:
-            if "expected BlockSequence" not in str(e):
-                raise
-            route = _make_route(normalized)
-            op = _core.Op.replace(value)
-            return doc._apply_patches([_core.Patch(route=route, operation=op)])
+            return self
+        return self._apply_patches(patches)
 
     def find_index(self, *keys: KeyPart, where: dict[str, Any]) -> int | None:
         """Return the index of the first list item matching all key/value pairs.

src/yamltrip/editor.py

@@ -57,6 +57,7 @@ def __exit__(
         made between ``__enter__`` and ``__exit__`` but is not atomic and
         cannot guard against concurrent writes during the write itself.
         """
+        del exc_val, exc_tb
         if exc_type is None and self._document is not None:
             current_source = self._path.read_text(encoding="utf-8")
             if current_source != self._original_source:
@@ -152,7 +153,7 @@ def sync(self, *keys: KeyPart, value: Any) -> None:
         self._document = self.document.sync(*keys, value=value)
 
     def merge(self, *keys: KeyPart, value: Any) -> None:
-        """Merge a value into the mapping at path without removing extra keys."""
+        """Merge or replace the value at path, depending on node type."""
         self._document = self.document.merge(*keys, value=value)
 
     def find_index(self, *keys: KeyPart, where: dict[str, Any]) -> int | None:

src/yamltrip/sync.py

@@ -3,6 +3,7 @@
 from __future__ import annotations
 
 from difflib import SequenceMatcher
+from enum import Enum
 from typing import TYPE_CHECKING, Any
 
 from yamltrip import _core
@@ -11,12 +12,22 @@
     from yamltrip._types import KeyPart
 
 
+class DiffMode(Enum):
+    """Controls how _compute_patches diffs values."""
+
+    SYNC = "sync"
+    """Exact sync: remove extra keys, diff lists element-wise."""
+
+    MERGE = "merge"
+    """Additive merge: keep extra keys, replace lists entirely."""
+
+
 def _compute_patches(
     old_value: Any,
     new_value: Any,
     path: tuple[KeyPart, ...],
     *,
-    remove_extra: bool = True,
+    mode: DiffMode = DiffMode.SYNC,
 ) -> list[_core.Patch]:
     """Compute minimal patches to transform old_value into new_value at path."""
     if old_value == new_value:
@@ -26,14 +37,13 @@ def _compute_patches(
     new_is_dict = isinstance(new_value, dict)
 
     if old_is_dict and new_is_dict:
-        return _diff_mappings(old_value, new_value, path, remove_extra=remove_extra)
+        return _diff_mappings(old_value, new_value, path, mode=mode)
 
     old_is_list = isinstance(old_value, list)
     new_is_list = isinstance(new_value, list)
 
     if old_is_list and new_is_list:
-        if not remove_extra:
-            # merge semantics: lists replace entirely (no element-wise diff)
+        if mode is DiffMode.MERGE:
             route = _core.Route(list(path))
             return [_core.Patch(route=route, operation=_core.Op.replace(new_value))]
         return _diff_lists(old_value, new_value, path)
@@ -49,7 +59,7 @@ def _diff_mappings(
     new: dict[str, Any],
     path: tuple[KeyPart, ...],
     *,
-    remove_extra: bool = True,
+    mode: DiffMode = DiffMode.SYNC,
 ) -> list[_core.Patch]:
     """Diff two mappings and return patches."""
     patches: list[_core.Patch] = []
@@ -58,7 +68,7 @@ def _diff_mappings(
     for key in new:
         if key in old:
             child_patches = _compute_patches(
-                old[key], new[key], (*path, key), remove_extra=remove_extra
+                old[key], new[key], (*path, key), mode=mode
             )
             patches.extend(child_patches)
         else:
@@ -68,7 +78,7 @@ def _diff_mappings(
             patches.append(_core.Patch(route=route, operation=op))
 
     # Keys in old not in new — remove
-    if remove_extra:
+    if mode is DiffMode.SYNC:
         for key in old:
             if key not in new:
                 route = _core.Route([*path, key])