Merge pull request #155 from Tobias-Fischer/fix/sort-vinca-lists-duplicate-conditions

fix: reject duplicate if-block conditions within the same list key

Author: traversaro

vinca/sort_vinca_lists.py

@@ -5,6 +5,15 @@
 Conditional blocks (`- if: ... then: [...]`) stay at the end; their inner
 `then:` lists are also sorted.
 
+Also validates that each condition (e.g. "win", "not win", "linux") appears
+in at most one `- if:` block per top-level list key. Two separate blocks for
+the same condition are never wrong on their own, but they're a standing
+foot-gun: a future addition can land in the "wrong" one by accident, and
+nothing merges the two, so the same package can end up skipped from one
+angle and not the other depending on which block someone edits. Raises
+DuplicateConditionError (exit 1) rather than silently merging them, since
+merging by hand needs a human to reconcile any per-item comments correctly.
+
 Usage:
   vinca-sort-vinca-lists [FILE]
   vinca-sort-vinca-lists --check [FILE]
@@ -27,12 +36,19 @@
 RE_SIMPLE_ITEM = re.compile(r"^  - (\S.*)$")
 # Regex for the start of a conditional block: "  - if: ..."
 RE_IF_BLOCK = re.compile(r"^  - if:")
+# Regex capturing the condition text of a "  - if: <condition>" line
+RE_IF_CONDITION = re.compile(r"^  - if:\s*(.+?)\s*$")
 # Regex for a then-list item inside a conditional block: "      - value"
 RE_THEN_ITEM = re.compile(r"^      - (\S.*)$")
 # Regex for a top-level key
 RE_TOP_KEY = re.compile(r"^(\S+):")
 
 
+class DuplicateConditionError(ValueError):
+    """Raised when the same `- if:` condition appears in more than one block
+    under the same top-level list key."""
+
+
 def _sort_key(line: str) -> str:
     """Extract sortable value from a list item line (lowercase, ignore comments)."""
     m = RE_SIMPLE_ITEM.match(line) or RE_THEN_ITEM.match(line)
@@ -57,6 +73,7 @@ def sort_vinca_lists(path: Path) -> bool:
         # Check if this line starts a target list key
         m = RE_TOP_KEY.match(line)
         if m and m.group(1) in LISTS_TO_SORT:
+            list_key = m.group(1)
             result.append(line)
             i += 1
 
@@ -136,6 +153,25 @@ def sort_vinca_lists(path: Path) -> bool:
             if current_if_block is not None:
                 if_blocks.append(current_if_block)
 
+            # Reject duplicate conditions: two separate "- if:" blocks for the
+            # same condition under the same list key. See module docstring.
+            seen_conditions = {}
+            for block in if_blocks:
+                cond_match = RE_IF_CONDITION.match(block[0])
+                if not cond_match:
+                    continue
+                condition = cond_match.group(1)
+                if condition in seen_conditions:
+                    raise DuplicateConditionError(
+                        f"{list_key}: condition {condition!r} appears in more "
+                        f"than one '- if:' block. Merge them into a single "
+                        f"block (each item may need its own comment moved "
+                        f"inline first, since sorting the merged then: list "
+                        f"can otherwise separate a standalone comment from "
+                        f"the item it was meant to describe)."
+                    )
+                seen_conditions[condition] = True
+
             # Sort simple items
             sorted_simple = sorted(simple_items, key=_sort_key)
             if sorted_simple != simple_items:
@@ -221,14 +257,20 @@ def main():
         print(f"ERROR: {args.file} not found", file=sys.stderr)
         sys.exit(1)
 
-    if args.check:
-        with tempfile.NamedTemporaryFile(suffix=".yaml", delete=False) as tf:
-            tmp = Path(tf.name)
-        shutil.copy2(args.file, tmp)
-        changed = sort_vinca_lists(tmp)
-        tmp.unlink(missing_ok=True)
-    else:
-        changed = sort_vinca_lists(args.file)
+    try:
+        if args.check:
+            with tempfile.NamedTemporaryFile(suffix=".yaml", delete=False) as tf:
+                tmp = Path(tf.name)
+            shutil.copy2(args.file, tmp)
+            try:
+                changed = sort_vinca_lists(tmp)
+            finally:
+                tmp.unlink(missing_ok=True)
+        else:
+            changed = sort_vinca_lists(args.file)
+    except DuplicateConditionError as e:
+        print(f"ERROR: {args.file}: {e}", file=sys.stderr)
+        sys.exit(1)
 
     status = ("UNSORTED" if args.check else "SORTED") if changed else "OK"
     print(f"{status}: {args.file}")

vinca/test_sort_vinca_lists.py

@@ -1,6 +1,8 @@
 """Tests for the vinca.yaml list sorter."""
 
-from vinca.sort_vinca_lists import sort_vinca_lists
+import pytest
+
+from vinca.sort_vinca_lists import DuplicateConditionError, sort_vinca_lists
 
 BASE = """packages_select_by_deps:
   - alpha
@@ -96,3 +98,64 @@ def test_adjacent_if_blocks_separated_by_comment_stay_isolated(tmp_path):
     first_block, second_block = path.read_text().split("- if: linux and not aarch64")
     assert "- webots_ros2" not in first_block
     assert "- webots_ros2" in second_block
+
+
+def test_duplicate_condition_in_same_list_raises(tmp_path):
+    # Two non-adjacent "- if: win" blocks under the same list key, separated
+    # by unrelated content -- not the adjacency bug from the previous fix,
+    # just two blocks that should have been one all along.
+    content = """packages_select_by_deps:
+  - if: win
+    then:
+      - alpha
+
+  - unrelated_package
+
+  - if: win
+    then:
+      - bravo
+"""
+    path = tmp_path / "vinca.yaml"
+    path.write_text(content)
+
+    with pytest.raises(
+        DuplicateConditionError, match=r"packages_select_by_deps.*'win'"
+    ):
+        sort_vinca_lists(path)
+
+
+def test_duplicate_condition_scoped_per_list_key(tmp_path):
+    # The same condition appearing once in each of two different list keys
+    # is fine -- duplication is only a problem within a single list key.
+    content = """packages_skip_by_deps:
+  - if: win
+    then:
+      - alpha
+
+packages_select_by_deps:
+  - if: win
+    then:
+      - bravo
+"""
+    path = tmp_path / "vinca.yaml"
+    path.write_text(content)
+
+    # Should not raise.
+    sort_vinca_lists(path)
+
+
+def test_different_conditions_do_not_raise(tmp_path):
+    content = """packages_select_by_deps:
+  - if: win
+    then:
+      - alpha
+
+  - if: not win
+    then:
+      - bravo
+"""
+    path = tmp_path / "vinca.yaml"
+    path.write_text(content)
+
+    # Should not raise.
+    sort_vinca_lists(path)