diff --git a/milestones/README.md b/milestones/README.md index 9114321..d820d9d 100644 --- a/milestones/README.md +++ b/milestones/README.md @@ -5,10 +5,13 @@ 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:** **[wizard-path-ref.md](wizard-path-ref.md)** — -wizard `open-bundle` `path` and `tool/update` `ref` must be JSON strings. +**Active:** **[timing-start-end.md](timing-start-end.md)** — +`timing.json` word/segment `start` / `end` must be JSON numbers. **Shipped:** +- **[wizard-path-ref.md](wizard-path-ref.md)** — + wizard `open-bundle` `path` and `tool/update` `ref` must be JSON strings + (#133). - **[wizard-json-parse.md](wizard-json-parse.md)** — wizard JSON bodies must parse; garbage JSON must not look like `{}` (#132). diff --git a/milestones/timing-start-end.md b/milestones/timing-start-end.md new file mode 100644 index 0000000..c1a023a --- /dev/null +++ b/milestones/timing-start-end.md @@ -0,0 +1,39 @@ +# Milestone: timing.json word/segment start and end must be numbers + +**Status:** Active +**PR:** [#134](https://github.com/jmjava/documentation-generator/pull/134) +**Depends on:** `milestones/timing-inner-lists.md` (PR #127), +`milestones/bootstrap-timing-helpers.md` (PR #130), +`milestones/wizard-path-ref.md` (PR #133) + +## Problem + +PR #127 required ``words`` / ``segments`` to be arrays of objects. Each row +still used ``float(row.get("start", 0.0))``. + +``start: true`` became ``1.0`` because ``bool`` is a subclass of ``int``. +A missing ``start`` waited until ``0.0`` and dumped the board. A string +``"0.5"`` looked like a timestamp instead of a type error. + +## Goal + +When a ``words`` / ``segments`` row is present, ``start`` and ``end`` must +be JSON numbers (``int`` or ``float``, not ``bool``). Missing / null / string +rows fail closed in ``load_bundle_timing`` and in Manim ``_load_timing`` / +``_load_timing_words``. Empty ``[]`` / missing / null lists stay allowed. + +``scene-compile`` refreshes bootstrap helpers that only typed objects. + +## Done when + +- [x] ``load_bundle_timing`` rejects missing / null / bool / string ``start`` / ``end`` +- [x] Bootstrap loaders reject the same at Manim render +- [x] Stale object-only loaders are refreshed +- [x] `ruff check src/ tests/` +- [x] `pytest tests/` (766 passed, 1 skipped) +- [x] `docgen benchmark` (helper change; meets baseline, no bump) + +## Out of scope + +- ``wait_until_word`` still swallows ``TypeError`` / ``ValueError`` after load +- Requiring ``end >= start`` diff --git a/milestones/wizard-path-ref.md b/milestones/wizard-path-ref.md index 0871bad..89beedb 100644 --- a/milestones/wizard-path-ref.md +++ b/milestones/wizard-path-ref.md @@ -1,6 +1,6 @@ # Milestone: wizard open-bundle path and tool/update ref must be strings -**Status:** Active +**Status:** Shipped **PR:** [#133](https://github.com/jmjava/documentation-generator/pull/133) **Depends on:** `milestones/wizard-json-parse.md` (PR #132) diff --git a/src/docgen/manim_scene_support.py b/src/docgen/manim_scene_support.py index 2f52bfa..5454012 100644 --- a/src/docgen/manim_scene_support.py +++ b/src/docgen/manim_scene_support.py @@ -171,6 +171,20 @@ def _load_timing(segment_key: str) -> list[dict]: raise TypeError( f"timing.json[{segment_key!r}].segments[{i}] must be a JSON object, not {kind}" ) + for time_key in ("start", "end"): + if time_key not in item or item[time_key] is None: + tkind = "missing" if time_key not in item else "null" + raise TypeError( + f"timing.json[{segment_key!r}].segments[{i}].{time_key} " + f"must be a JSON number, not {tkind}" + ) + val = item[time_key] + if isinstance(val, bool) or not isinstance(val, (int, float)): + tkind = type(val).__name__ + raise TypeError( + f"timing.json[{segment_key!r}].segments[{i}].{time_key} " + f"must be a JSON number, not {tkind}" + ) return list(segs) @@ -204,6 +218,20 @@ def _load_timing_words(segment_key: str) -> list[dict]: raise TypeError( f"timing.json[{segment_key!r}].words[{i}] must be a JSON object, not {kind}" ) + for time_key in ("start", "end"): + if time_key not in item or item[time_key] is None: + tkind = "missing" if time_key not in item else "null" + raise TypeError( + f"timing.json[{segment_key!r}].words[{i}].{time_key} " + f"must be a JSON number, not {tkind}" + ) + val = item[time_key] + if isinstance(val, bool) or not isinstance(val, (int, float)): + tkind = type(val).__name__ + raise TypeError( + f"timing.json[{segment_key!r}].words[{i}].{time_key} " + f"must be a JSON number, not {tkind}" + ) return list(words) @@ -1297,7 +1325,11 @@ def helper_needs_refresh(tree: ast.AST, name: str) -> bool: if name in {"_load_timing", "_load_timing_words"} and isinstance( node, ast.FunctionDef ) and node.name == name: - return "must be a JSON object" not in ast.unparse(node) + src = ast.unparse(node) + return ( + "must be a JSON object" not in src + or "must be a JSON number" not in src + ) if name == "_image" and isinstance(node, ast.FunctionDef) and node.name == "_image": return False return False diff --git a/src/docgen/timestamps.py b/src/docgen/timestamps.py index 47113f0..1f82e6a 100644 --- a/src/docgen/timestamps.py +++ b/src/docgen/timestamps.py @@ -32,10 +32,27 @@ def _json_kind(value: Any) -> str: return "null" if value is None else type(value).__name__ +def _json_number_kind(row: dict[str, Any], time_key: str) -> str | None: + """Return a kind label when *time_key* is not a JSON number; ``None`` when ok. + + ``bool`` is a subclass of ``int``: ``start: true`` used to become ``1.0s``. + """ + if time_key not in row: + return "missing" + value = row[time_key] + if value is None: + return "null" + if isinstance(value, bool) or not isinstance(value, (int, float)): + return _json_kind(value) + return None + + def _require_timing_object_list( path_name: str, stem: str, payload: dict[str, Any], key: str ) -> None: - """Require ``words`` / ``segments`` (when present and not null) to be object arrays.""" + """Require ``words`` / ``segments`` (when present and not null) to be object arrays + whose ``start`` / ``end`` are JSON numbers. + """ if key not in payload: return value = payload[key] @@ -51,15 +68,23 @@ def _require_timing_object_list( f"{path_name}[{stem!r}].{key}[{i}] must be a JSON object, " f"not {_json_kind(item)}" ) + for time_key in ("start", "end"): + kind = _json_number_kind(item, time_key) + if kind is not None: + raise TimestampError( + f"{path_name}[{stem!r}].{key}[{i}].{time_key} must be a JSON " + f"number, not {kind}" + ) def load_bundle_timing(config: "Config") -> dict[str, Any]: """Load ``animations/timing.json``. A missing file is ``{}``. Corrupt JSON, a non-object root, a non-object - per-stem value, or a present ``words`` / ``segments`` field that is not an - array of objects raises :class:`TimestampError` so compile/validate cannot - treat garbage as empty ``words``. + per-stem value, a present ``words`` / ``segments`` field that is not an + array of objects, or a row whose ``start`` / ``end`` is not a JSON number + raises :class:`TimestampError` so compile/validate cannot treat garbage as + empty ``words`` or wait until ``0.0``. """ path = config.animations_dir / "timing.json" if not path.is_file(): @@ -168,7 +193,8 @@ def extract_all(self, engine: str | None = None) -> None: Successful runs **merge** stems into the existing file (same as the wizard per-segment timestamps step) so extra keys not in ``segments.all`` are not wiped. Corrupt JSON, a non-object root, a - non-object stem, or a non-array ``words`` / ``segments`` field raises + non-object stem, a non-array ``words`` / ``segments`` field, or a row + whose ``start`` / ``end`` is not a JSON number raises :class:`TimestampError` and the file is not rewritten. """ chosen = self.resolve_engine(engine) diff --git a/tests/test_manim_scene_support.py b/tests/test_manim_scene_support.py index 1a77802..eb354df 100644 --- a/tests/test_manim_scene_support.py +++ b/tests/test_manim_scene_support.py @@ -810,6 +810,21 @@ def test_load_timing_helpers_accept_object_rows(tmp_path: Path) -> None: assert ns["_load_timing_words"]("missing") == [] +def test_load_timing_helpers_reject_non_numeric_start_end(tmp_path: Path) -> None: + ns = _exec_timing_loaders( + tmp_path, + {"01-x": {"words": [{"word": "hi", "start": True, "end": 0.1}]}}, + ) + with pytest.raises(TypeError, match=r"\.words\[0\].start must be a JSON number, not bool"): + ns["_load_timing_words"]("01-x") + ns2 = _exec_timing_loaders( + tmp_path, + {"01-x": {"segments": [{"text": "hi", "start": 0.0}]}}, + ) + with pytest.raises(TypeError, match=r"\.segments\[0\].end must be a JSON number, not missing"): + ns2["_load_timing"]("01-x") + + def test_refresh_bootstrap_helpers_upgrades_stale_timing_loaders(tmp_path: Path) -> None: p = tmp_path / "scenes.py" p.write_text( @@ -828,9 +843,26 @@ def test_refresh_bootstrap_helpers_upgrades_stale_timing_loaders(tmp_path: Path) assert set(changed) == {"_load_timing", "_load_timing_words"} text = p.read_text(encoding="utf-8") assert "must be a JSON object" in text + assert "must be a JSON number" in text assert "data.get(segment_key, {}).get('segments'" not in text +def test_refresh_bootstrap_helpers_upgrades_object_only_timing_loaders(tmp_path: Path) -> None: + """#130 bodies typed stems/rows but still coerced missing start to 0.0.""" + p = tmp_path / "scenes.py" + p.write_text( + "from manim import *\n" + "def _load_timing(segment_key):\n" + " raise TypeError('timing.json[k] must be a JSON object')\n" + "def _load_timing_words(segment_key):\n" + " raise TypeError('timing.json[k] must be a JSON object')\n", + encoding="utf-8", + ) + changed = refresh_bootstrap_helpers(p) + assert set(changed) == {"_load_timing", "_load_timing_words"} + assert "must be a JSON number" in p.read_text(encoding="utf-8") + + def test_ensure_bootstrap_refreshes_stale_helpers(tmp_path: Path) -> None: p = tmp_path / "scenes.py" diff --git a/tests/test_scene_asset_validate.py b/tests/test_scene_asset_validate.py index d6b9659..3e4fb66 100644 --- a/tests/test_scene_asset_validate.py +++ b/tests/test_scene_asset_validate.py @@ -227,6 +227,19 @@ def _load_timing_words(segment_key): assert any("_load_timing_words is stale" in i for i in issues) +def test_helper_api_flags_object_only_timing_loaders() -> None: + stale = ''' +MANIM_FONT = "Liberation Sans" +def _load_timing(segment_key): + raise TypeError("must be a JSON object") +def _load_timing_words(segment_key): + raise TypeError("must be a JSON object") +''' + issues = helper_api_violations(stale) + assert any("_load_timing is stale" in i for i in issues) + assert any("_load_timing_words is stale" in i for i in issues) + + def test_helper_api_clean_for_current_bootstrap() -> None: assert helper_api_violations(BOOTSTRAP_HEADER) == [] @@ -399,3 +412,23 @@ def test_non_array_timing_words_is_reported(tmp_path: Path) -> None: assert any("timing.json['01-x'].words must be a JSON array, not str" in i for i in issues) +def test_non_numeric_timing_start_is_reported(tmp_path: Path) -> None: + cfg = _bundle(tmp_path) + specs = cfg.animations_dir / "specs" + specs.mkdir(parents=True, exist_ok=True) + (specs / "01-x.scene.yaml").write_text( + yaml.dump(_spec([_box("Alpha")])), + encoding="utf-8", + ) + (cfg.animations_dir / "timing.json").write_text( + json.dumps({"01-x": {"words": [{"word": "hi", "start": True, "end": 0.1}]}}) + + "\n", + encoding="utf-8", + ) + issues = scene_asset_violations_for_segment(cfg, "01") + assert any( + "timing.json['01-x'].words[0].start must be a JSON number, not bool" in i + for i in issues + ) + + diff --git a/tests/test_timestamps_local.py b/tests/test_timestamps_local.py index 1996b51..ae77aac 100644 --- a/tests/test_timestamps_local.py +++ b/tests/test_timestamps_local.py @@ -194,7 +194,12 @@ def test_extract_all_preserves_extra_timing_stems(self, cfg, monkeypatch) -> Non out = cfg.animations_dir / "timing.json" out.parent.mkdir(parents=True, exist_ok=True) out.write_text( - json.dumps({"legacy-stem": {"text": "keep-me", "words": [{"word": "x"}]}}), + json.dumps({ + "legacy-stem": { + "text": "keep-me", + "words": [{"word": "x", "start": 0.0, "end": 0.1}], + } + }), encoding="utf-8", ) TimestampExtractor(cfg).extract_all() @@ -333,3 +338,49 @@ def test_extract_all_rejects_non_array_words_without_rewrite(self, cfg, monkeypa TimestampExtractor(cfg).extract_all() assert out.read_text(encoding="utf-8") == payload + def test_load_bundle_timing_rejects_non_numeric_start_end(self, cfg) -> None: + from docgen.timestamps import TimestampError, load_bundle_timing + + out = cfg.animations_dir / "timing.json" + out.parent.mkdir(parents=True, exist_ok=True) + cases = ( + ( + {"01-x": {"words": [{"word": "hi", "end": 0.1}]}}, + r"timing.json\['01-x'\].words\[0\].start must be a JSON number, not missing", + ), + ( + {"01-x": {"words": [{"word": "hi", "start": None, "end": 0.1}]}}, + r"timing.json\['01-x'\].words\[0\].start must be a JSON number, not null", + ), + ( + {"01-x": {"words": [{"word": "hi", "start": True, "end": 0.1}]}}, + r"timing.json\['01-x'\].words\[0\].start must be a JSON number, not bool", + ), + ( + {"01-x": {"words": [{"word": "hi", "start": "0.0", "end": 0.1}]}}, + r"timing.json\['01-x'\].words\[0\].start must be a JSON number, not str", + ), + ( + {"01-x": {"segments": [{"text": "hi", "start": 0.0}]}}, + r"timing.json\['01-x'\].segments\[0\].end must be a JSON number, not missing", + ), + ) + for payload, match in cases: + out.write_text(json.dumps(payload), encoding="utf-8") + with pytest.raises(TimestampError, match=match): + load_bundle_timing(cfg) + + def test_load_bundle_timing_accepts_numeric_start_end(self, cfg) -> None: + from docgen.timestamps import load_bundle_timing + + out = cfg.animations_dir / "timing.json" + out.parent.mkdir(parents=True, exist_ok=True) + payload = { + "01-x": { + "words": [{"word": "hi", "start": 0, "end": 0.25}], + "segments": [{"text": "hi", "start": 0.0, "end": 1}], + } + } + out.write_text(json.dumps(payload), encoding="utf-8") + assert load_bundle_timing(cfg) == payload +