diff --git a/vinca/sort_vinca_lists.py b/vinca/sort_vinca_lists.py index 2db7d4e..8de8dc5 100644 --- a/vinca/sort_vinca_lists.py +++ b/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: " 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}") diff --git a/vinca/test_sort_vinca_lists.py b/vinca/test_sort_vinca_lists.py index 44777c6..3febd5a 100644 --- a/vinca/test_sort_vinca_lists.py +++ b/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)