From feaf6275a8da21890c252fba196a8de8c43ce594 Mon Sep 17 00:00:00 2001 From: Cursor Agent Date: Mon, 7 Sep 2026 22:33:40 +0000 Subject: [PATCH 1/2] Honor empty segments.all in yaml-generate An explicit all: [] was treated as missing via `all or default`, so discover_visual_map and manim.scenes sync walked default ids even though Config.segments_all is empty. Use all whenever the key is present, including an empty list. Co-authored-by: jmjava --- milestones/README.md | 9 +++-- milestones/concat-ffmpeg-timeout.md | 2 +- milestones/empty-segments-all.md | 39 ++++++++++++++++++++ src/docgen/yaml_generate.py | 30 +++++++++++----- tests/test_yaml_generate.py | 55 +++++++++++++++++++++++++++++ 5 files changed, 123 insertions(+), 12 deletions(-) create mode 100644 milestones/empty-segments-all.md diff --git a/milestones/README.md b/milestones/README.md index 6b0c270..71ea156 100644 --- a/milestones/README.md +++ b/milestones/README.md @@ -5,11 +5,14 @@ repositories that install `docgen` and maintain their own demo bundle. The library no longer ships an in-repo dogfood; consumers are the integration test of record. -**Active:** **[concat-ffmpeg-timeout.md](concat-ffmpeg-timeout.md)** — -concat must not leave a truncated full-demo mp4 after ffmpeg -timeout or failure. +**Active:** **[empty-segments-all.md](empty-segments-all.md)** — +explicit empty `segments.all: []` must not fall through to +`segments.default` in yaml-generate. **Shipped:** +- **[concat-ffmpeg-timeout.md](concat-ffmpeg-timeout.md)** — + concat must not leave a truncated full-demo mp4 after ffmpeg + timeout or failure (#123). - **[generation-zero-values.md](generation-zero-values.md)** — `temperature: 0` / `max_whisper_segment_text_chars: 0` must not be replaced by ``or`` defaults (#122). diff --git a/milestones/concat-ffmpeg-timeout.md b/milestones/concat-ffmpeg-timeout.md index 4925974..973f333 100644 --- a/milestones/concat-ffmpeg-timeout.md +++ b/milestones/concat-ffmpeg-timeout.md @@ -1,6 +1,6 @@ # Milestone: concat must not leave a truncated ffmpeg output -**Status:** Active +**Status:** Shipped **PR:** [#123](https://github.com/jmjava/documentation-generator/pull/123) **Depends on:** `milestones/compose-ffmpeg-timeout.md` (PR #121), `milestones/generation-zero-values.md` (PR #122) diff --git a/milestones/empty-segments-all.md b/milestones/empty-segments-all.md new file mode 100644 index 0000000..a9c1484 --- /dev/null +++ b/milestones/empty-segments-all.md @@ -0,0 +1,39 @@ +# Milestone: empty segments.all must not fall through to default + +**Status:** Active +**PR:** (pending) +**Depends on:** `milestones/concat-ffmpeg-timeout.md` (PR #123), +`milestones/yaml-generate-mappings.md` (PR #91) + +## Problem + +``Config.segments_all`` honors an explicit empty ``segments.all: []`` +(no segments). ``yaml-generate`` used ``all or default``, so an empty +list was treated as missing: + +1. ``discover_visual_map`` assigned Manim classes to ``default`` ids + and rewrote ``visual_map``. +2. ``manim.scenes`` sync walked ``default`` instead of empty ``all``. +3. ``--list-gaps`` treated ``default`` ids as already in ``all``. + +## Goal + +When ``segments.all`` is present (including ``[]``), yaml-generate +must use that list. Fall back to ``segments.default`` only when +``all`` is missing or null — the same rule as ``Config.segments_all``. + +## Done when + +- [ ] Empty ``all: []`` does not discover/sync from ``default``. +- [ ] Missing ``all`` still uses ``default``. +- [ ] Tests cover segments_in_config, discover_visual_map, and + manim.scenes sync. +- [ ] `ruff check src/ tests/` +- [ ] `pytest tests/` +- [ ] `docgen benchmark` (no clock change; meets baseline) + +## Out of scope + +- Changing how hint ``docgen.segment.create`` appends into existing + lists. +- Coercing non-list ``all`` values (Config already rejects those). diff --git a/src/docgen/yaml_generate.py b/src/docgen/yaml_generate.py index f398da4..f75a415 100644 --- a/src/docgen/yaml_generate.py +++ b/src/docgen/yaml_generate.py @@ -99,12 +99,28 @@ def segments_in_config(raw: dict[str, Any]) -> set[str]: seg = raw.get("segments") or {} if not isinstance(seg, dict): return set() - all_ids = seg.get("all") or seg.get("default") or [] - if not isinstance(all_ids, list): - return set() + all_ids = _segment_all_ids(seg) return {str(x) for x in all_ids} +def _segment_all_ids(seg: dict[str, Any]) -> list[Any]: + """Return ``segments.all`` when the key is present (including an empty list). + + Fall back to ``segments.default`` only when ``all`` is missing or null. + ``all: [] or default`` used to treat an explicit empty ``all`` as missing + and rewrite ``visual_map`` / ``manim.scenes`` from ``default``. + """ + if "all" in seg and seg.get("all") is not None: + raw_ids = seg.get("all") + else: + raw_ids = seg.get("default") + if raw_ids is None: + return [] + if not isinstance(raw_ids, list): + return [] + return list(raw_ids) + + def narration_not_in_segments(raw: dict[str, Any], narration_dir: Path) -> list[tuple[str, str]]: """Narration stems whose numeric id is missing from ``segments.all``.""" have = segments_in_config(raw) @@ -760,8 +776,8 @@ def discover_visual_map(raw: dict[str, Any], cfg: "Config") -> list[str]: if seg_block is None: return [] seg_block = _require_yaml_mapping(seg_block, label="segments") - all_ids = seg_block.get("all") or seg_block.get("default") or [] - if not isinstance(all_ids, list) or not all_ids: + all_ids = _segment_all_ids(seg_block) + if not all_ids: return [] scenes_py = cfg.animations_dir / "scenes.py" @@ -855,9 +871,7 @@ def _sync_manim_scenes_from_visual_map(raw: dict[str, Any]) -> list[str]: all_ids: list[Any] = [] else: seg_block = _require_yaml_mapping(seg_block, label="segments") - all_ids = seg_block.get("all") or seg_block.get("default") or [] - if not isinstance(all_ids, list): - all_ids = [] + all_ids = _segment_all_ids(seg_block) scenes: list[str] = [] seen: set[str] = set() for sid in all_ids: diff --git a/tests/test_yaml_generate.py b/tests/test_yaml_generate.py index 980ac54..8fc3a54 100644 --- a/tests/test_yaml_generate.py +++ b/tests/test_yaml_generate.py @@ -155,6 +155,16 @@ def test_segments_in_config_reads_all_alias(tmp_path: Path) -> None: assert segments_in_config(raw) == {"03", "04"} +def test_segments_in_config_empty_all_does_not_fall_through_to_default() -> None: + raw = {"segments": {"all": [], "default": ["01", "02"]}} + assert segments_in_config(raw) == set() + + +def test_segments_in_config_missing_all_uses_default() -> None: + raw = {"segments": {"default": ["01", "02"]}} + assert segments_in_config(raw) == {"01", "02"} + + def test_manim_scene_class_names_in_order(tmp_path: Path) -> None: ad = tmp_path / "animations" ad.mkdir() @@ -212,6 +222,51 @@ def test_discover_visual_map_manim_classes_in_order(tmp_path: Path) -> None: assert raw["visual_map"]["02"]["scene"] == "SecondScene" +def test_discover_visual_map_empty_all_does_not_use_default(tmp_path: Path) -> None: + (tmp_path / "animations").mkdir() + (tmp_path / "animations" / "scenes.py").write_text( + "class IntroScene(Scene):\n pass\n", + encoding="utf-8", + ) + raw = { + "repo_root": ".", + "dirs": { + "narration": "narration", + "audio": "audio", + "animations": "animations", + "recordings": "recordings", + }, + "segments": {"all": [], "default": ["01"]}, + "segment_names": {"01": "01-intro"}, + "visual_map": {}, + } + (tmp_path / "docgen.yaml").write_text(yaml.dump(raw), encoding="utf-8") + cfg = Config.from_yaml(tmp_path / "docgen.yaml") + assert discover_visual_map(raw, cfg) == [] + assert raw["visual_map"] == {} + + +def test_merge_defaults_empty_all_does_not_sync_manim_scenes_from_default( + tmp_path: Path, +) -> None: + raw = { + "repo_root": ".", + "dirs": { + "narration": "narration", + "audio": "audio", + "animations": "animations", + "recordings": "recordings", + }, + "segments": {"all": [], "default": ["01"]}, + "visual_map": {"01": {"type": "manim", "scene": "IntroScene"}}, + "manim": {"scenes": ["IntroScene"]}, + } + (tmp_path / "animations").mkdir() + cfg = _cfg(tmp_path, raw) + merge_defaults(raw, cfg) + assert raw["manim"]["scenes"] == [] + + def test_discover_visual_map_manim_assigns_only_when_classes_available(tmp_path: Path) -> None: (tmp_path / "animations").mkdir() (tmp_path / "animations" / "scenes.py").write_text( From a206a25d0ad5b6de7d14e3851d1ccd6feb372f57 Mon Sep 17 00:00:00 2001 From: Cursor Agent Date: Mon, 7 Sep 2026 22:34:23 +0000 Subject: [PATCH 2/2] Mark empty-segments-all milestone gates as run ruff, pytest (713 passed, 1 skipped), and docgen benchmark all green. Co-authored-by: jmjava --- milestones/empty-segments-all.md | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/milestones/empty-segments-all.md b/milestones/empty-segments-all.md index a9c1484..6bda136 100644 --- a/milestones/empty-segments-all.md +++ b/milestones/empty-segments-all.md @@ -1,7 +1,7 @@ # Milestone: empty segments.all must not fall through to default **Status:** Active -**PR:** (pending) +**PR:** [#124](https://github.com/jmjava/documentation-generator/pull/124) **Depends on:** `milestones/concat-ffmpeg-timeout.md` (PR #123), `milestones/yaml-generate-mappings.md` (PR #91) @@ -24,13 +24,13 @@ must use that list. Fall back to ``segments.default`` only when ## Done when -- [ ] Empty ``all: []`` does not discover/sync from ``default``. -- [ ] Missing ``all`` still uses ``default``. -- [ ] Tests cover segments_in_config, discover_visual_map, and +- [x] Empty ``all: []`` does not discover/sync from ``default``. +- [x] Missing ``all`` still uses ``default``. +- [x] Tests cover segments_in_config, discover_visual_map, and manim.scenes sync. -- [ ] `ruff check src/ tests/` -- [ ] `pytest tests/` -- [ ] `docgen benchmark` (no clock change; meets baseline) +- [x] `ruff check src/ tests/` +- [x] `pytest tests/` (713 passed, 1 skipped) +- [x] `docgen benchmark` (no clock change; meets baseline) ## Out of scope