From 7260337b169bf566a8c3a301a21e6f1a936b2586 Mon Sep 17 00:00:00 2001 From: Brian O'Kelley Date: Mon, 9 Nov 2026 21:36:22 +0000 Subject: [PATCH 1/4] fix(codegen): reuse SignalListing.Range by shape, and fail loudly when a copy survives fix_signal_listing_range_subclasses matched the generated Range text, extra='forbid' and all. The generator moved to StrictFloat and new descriptions, so the match stopped deleting the duplicate classes in signals/get_signals_response.py and core/wholesale_feed_event.py, and still printed success. Its import rewrite still fired, and remove_imports_shadowed_by_a_local_class then undid it, so the post-generation manifest saw both passes firing. adcp#8866 opens core/signal-listing.json#/properties/range, which would have kept it missing. The pass now finds each copy by name and compares it with SignalListing.Range by what it validates (bases, model_config, field types, constraints, defaults; not descriptions). On a match it binds Range to the shared class by assignment, so adcp.types.domains.signals.Range stays exported. It raises when a copy validates differently or a Range class survives. Regenerated on 3.3.0-beta.5: SignalListing subclasses now share SignalListing.Range, which also removes the parent/override type mismatch on `range`. Checked against adcp#8866's schema bundle (open range), where the pass applies to both modules. Co-Authored-By: Claude Opus 5.5 --- SCHEMA_DELTAS.md | 21 +-- scripts/post_generate_fixes.py | 167 +++++++++++++----- scripts/post_generation_manifest.json | 2 +- .../domains/core/wholesale_feed_event.py | 10 +- .../domains/signals/get_signals_response.py | 10 +- 5 files changed, 134 insertions(+), 76 deletions(-) diff --git a/SCHEMA_DELTAS.md b/SCHEMA_DELTAS.md index 6bc7f45a04..8a0f79ac0c 100644 --- a/SCHEMA_DELTAS.md +++ b/SCHEMA_DELTAS.md @@ -1,21 +1,8 @@ # Generated-types delta -## Files added - -- `core/match_key.py` — MatchKey -- `enums/match_key_use.py` — MatchKeyUse - ## Field changes -- `bundled/protocol/get_adcp_capabilities_response.py` - - **classes added**: BrandKitOverride11, PropertyType, SupportedMatchKey, Use, Vendor9 - - **classes removed**: Surface - - `MediaBuy`: `+supported_match_keys` -- `core/product_identity.py` - - `ProductIdentity`: `+match_keys` -- `protocol/get_adcp_capabilities_response.py` - - `MediaBuy`: `+supported_match_keys` -- `trusted_match/provider_registration.py` - - **classes added**: MatchKey, Use - - `TmpProviderRegistration1`: `+match_keys` - - `TmpProviderRegistration2`: `+match_keys` +- `core/wholesale_feed_event.py` + - **classes removed**: Range +- `signals/get_signals_response.py` + - **classes removed**: Range diff --git a/scripts/post_generate_fixes.py b/scripts/post_generate_fixes.py index e2c597f1de..1ab60413c0 100644 --- a/scripts/post_generate_fixes.py +++ b/scripts/post_generate_fixes.py @@ -5135,56 +5135,135 @@ def fix_mcp_webhook_operation_id_optional() -> None: print(" core/mcp_webhook_payload.py: made operation_id optional") +#: Generated modules whose signal redeclares ``range`` with its own copy of +#: ``SignalListing.Range``, keyed by path under ``OUTPUT_DIR``, with the module +#: each imports the shared class from. +_SIGNAL_LISTING_RANGE_REUSERS = { + ("signals", "get_signals_response.py"): "..core.signal_listing", + ("core", "wholesale_feed_event.py"): ".signal_listing", +} + + +class _DropDescriptions(ast.NodeTransformer): + """Strip ``description=`` from ``Field(...)`` and unwrap a bare ``Annotated[T]``.""" + + def visit_Call(self, node: ast.Call) -> ast.AST: + self.generic_visit(node) + if isinstance(node.func, ast.Name) and node.func.id == "Field": + node.keywords = [k for k in node.keywords if k.arg != "description"] + return node + + def visit_Subscript(self, node: ast.Subscript) -> ast.AST: + self.generic_visit(node) + if not (isinstance(node.value, ast.Name) and node.value.id == "Annotated"): + return node + elts = list(node.slice.elts) if isinstance(node.slice, ast.Tuple) else [node.slice] + kept = [ + e + for e in elts[1:] + if not ( + isinstance(e, ast.Call) + and isinstance(e.func, ast.Name) + and e.func.id == "Field" + and not e.args + and not e.keywords + ) + ] + if not kept: + return elts[0] + node.slice = ast.Tuple(elts=[elts[0], *kept], ctx=ast.Load()) + return node + + +def _range_shape(node: ast.ClassDef) -> str: + """What a generated ``Range`` class validates, with descriptions removed. + + The copies datamodel-code-generator emits differ from ``SignalListing.Range`` + only in field descriptions. Bases, ``model_config`` (so an open schema + differs from a closed one), field types, constraints and defaults all count. + """ + stripped = _DropDescriptions().visit(ast.parse(ast.unparse(node)).body[0]) + assert isinstance(stripped, ast.ClassDef) + stripped.body = [ + stmt + for stmt in stripped.body + if not (isinstance(stmt, ast.Expr) and isinstance(stmt.value, ast.Constant)) + ] + return ast.unparse(stripped) + + +def _top_level_class(tree: ast.Module, name: str) -> ast.ClassDef | None: + return next( + (node for node in tree.body if isinstance(node, ast.ClassDef) and node.name == name), + None, + ) + + def fix_signal_listing_range_subclasses() -> None: - """Reuse SignalListing.Range for generated subclasses that redeclare range.""" - replacements = { - OUTPUT_DIR - / "signals" - / "get_signals_response.py": [ - ( - "from ..core.signal_listing import SignalListing\n", - "from ..core.signal_listing import Range, SignalListing\n", - ), - ( - "\n\nclass Range(AdCPBaseModel):\n" - " model_config = ConfigDict(\n" - " extra='forbid',\n" - " )\n" - " min: Annotated[float, Field(description='Minimum value (inclusive)')]\n" - " max: Annotated[float, Field(description='Maximum value (inclusive)')]\n", - "", - ), - ], - OUTPUT_DIR - / "core" - / "wholesale_feed_event.py": [ - ( - "from .signal_listing import SignalListing\n", - "from .signal_listing import Range, SignalListing\n", - ), - ( - "\n\nclass Range(AdCPBaseModel):\n" - " model_config = ConfigDict(\n" - " extra='forbid',\n" - " )\n" - " min: float\n" - " max: float\n", - "", - ), - ], - } + """Reuse SignalListing.Range where a generated signal redeclares ``range``. + + A signal that extends ``core/signal-listing.json`` gets its own ``Range`` + class from the generator, identical to ``SignalListing.Range`` but a + different type, so a ``SignalListing.Range`` does not validate as the + subclass's ``range``. Each copy is found by name and compared with + ``SignalListing.Range`` by what it validates, so the pass keeps working when + the generator changes descriptions, scalar types or the schema's + ``additionalProperties``. It used to match the generated text, ``extra='forbid'`` + and all, and printed "not needed" once that text drifted. + + Raises when a copy no longer matches ``SignalListing.Range``, or when a + ``Range`` class would survive the pass: deleting a class that validates + differently changes the wire contract, and keeping a duplicate is the + silent miss this guards against. + """ + listing_path = OUTPUT_DIR / "core" / "signal_listing.py" + if not listing_path.exists(): + print(" signal_listing.py not found (skipping SignalListing.Range reuse)") + return + listing = _top_level_class(ast.parse(listing_path.read_text()), "Range") fixed = 0 - for path, path_replacements in replacements.items(): + for parts, module in _SIGNAL_LISTING_RANGE_REUSERS.items(): + path = OUTPUT_DIR.joinpath(*parts) + relative = "/".join(parts) if not path.exists(): + print(f" {relative} not found (skipping SignalListing.Range reuse)") continue source = path.read_text() - updated = source - for old, new in path_replacements: - updated = updated.replace(old, new, 1) - if updated != source: - path.write_text(updated) - fixed += 1 + local = _top_level_class(ast.parse(source), "Range") + if local is None: + continue + if listing is None: + raise RuntimeError( + f"{relative}: declares Range but core/signal_listing.py no longer does" + ) + if _range_shape(local) != _range_shape(listing): + raise RuntimeError( + f"{relative}: Range no longer matches SignalListing.Range, so it cannot be " + f"reused; revisit fix_signal_listing_range_subclasses.\n" + f" local: {_range_shape(local)!r}\n" + f" listing: {_range_shape(listing)!r}" + ) + + # Bind the shared class by assignment rather than import, so the module + # still declares ``Range``: ``scripts/consolidate_exports.py`` re-exports + # only declared names, and ``adcp.types.domains.signals.Range`` stays. + lines = source.splitlines(keepends=True) + assert local.end_lineno is not None + start = sum(map(len, lines[: local.lineno - 1])) + end = sum(map(len, lines[: local.end_lineno])) + source = source[:start] + "Range = _SignalListingRange\n" + source[end:] + imports = [n for n in ast.parse(source).body if isinstance(n, (ast.Import, ast.ImportFrom))] + if not imports or imports[-1].end_lineno is None: + raise RuntimeError(f"{relative}: no import block to add the Range import to") + lines = source.splitlines(keepends=True) + at = sum(map(len, lines[: imports[-1].end_lineno])) + source = source[:at] + f"from {module} import Range as _SignalListingRange\n" + source[at:] + + if _top_level_class(ast.parse(source), "Range") is not None: + raise RuntimeError(f"{relative}: a duplicate Range class survived the reuse pass") + path.write_text(source) + fixed += 1 if fixed: print(f" Reused SignalListing.Range in {fixed} subclass file(s)") diff --git a/scripts/post_generation_manifest.json b/scripts/post_generation_manifest.json index ba0f764a83..760931f4a2 100644 --- a/scripts/post_generation_manifest.json +++ b/scripts/post_generation_manifest.json @@ -75,7 +75,7 @@ "preserve_wire_text_on_normalizing_fields": true, "insert_task_message_markers": true, "point_integer_fields_at_the_schema_integer_type": true, - "remove_imports_shadowed_by_a_local_class": true, + "remove_imports_shadowed_by_a_local_class": "Its only target was core/wholesale_feed_event.py, where fix_signal_listing_range_subclasses added a Range import but missed the duplicate Range class it should have deleted. That pass now deletes the class, so no import is shadowed. Kept as a guard for the generator's import-and-redeclare pattern.", "remove_unused_pydantic_field_imports": true, "emit_open_asset_unions": true, "declare_root_schema_module_exports": true, diff --git a/src/adcp/types/domains/core/wholesale_feed_event.py b/src/adcp/types/domains/core/wholesale_feed_event.py index bd3985b1d7..b33e333a5b 100644 --- a/src/adcp/types/domains/core/wholesale_feed_event.py +++ b/src/adcp/types/domains/core/wholesale_feed_event.py @@ -1,6 +1,6 @@ # generated by datamodel-codegen: # filename: core/wholesale_feed_event.json -# timestamp: 2026-10-29T01:56:42+00:00 +# timestamp: 2026-11-09T21:22:22+00:00 from __future__ import annotations @@ -21,6 +21,7 @@ from . import signal_ref as signal_ref_1 from . import vendor_pricing_option from .signal_listing import SignalListing +from .signal_listing import Range as _SignalListingRange class EventType(StrEnum): @@ -90,12 +91,7 @@ class RemovalReason(StrEnum): policy_takedown = 'policy_takedown' -class Range(AdCPBaseModel): - model_config = ConfigDict( - extra='forbid', - ) - min: StrictFloat - max: StrictFloat +Range = _SignalListingRange class Payload(AdCPBaseModel): diff --git a/src/adcp/types/domains/signals/get_signals_response.py b/src/adcp/types/domains/signals/get_signals_response.py index 92984db949..124e75f29c 100644 --- a/src/adcp/types/domains/signals/get_signals_response.py +++ b/src/adcp/types/domains/signals/get_signals_response.py @@ -1,6 +1,6 @@ # generated by datamodel-codegen: # filename: signals/get_signals_response.json -# timestamp: 2026-10-31T11:55:18+00:00 +# timestamp: 2026-11-09T21:22:22+00:00 from __future__ import annotations @@ -29,14 +29,10 @@ from ..enums import restricted_attribute, signal_catalog_type from ..enums import signal_status as signal_status_1 from ..enums import signal_value_type +from ..core.signal_listing import Range as _SignalListingRange -class Range(AdCPBaseModel): - model_config = ConfigDict( - extra='forbid', - ) - min: Annotated[StrictFloat, Field(description='Minimum value, inclusive.')] - max: Annotated[StrictFloat, Field(description='Maximum value, inclusive.')] +Range = _SignalListingRange class Value(AdCPBaseModel): From 7b8520eabd871db2ad0bee8024eddf622a212707 Mon Sep 17 00:00:00 2001 From: Brian O'Kelley Date: Mon, 9 Nov 2026 21:42:41 +0000 Subject: [PATCH 2/4] test(codegen): cover SignalListing.Range reuse by shape, and repoint the public API snapshot SignalCoverageRange and adcp.types.domains.signals.Range now resolve to the shared core.signal_listing:Range, the intended effect of the reuse. Co-Authored-By: Claude Opus 5.5 --- tests/fixtures/public_api_snapshot.json | 4 +- tests/test_signal_listing_range_reuse.py | 272 +++++++++++++++++++++++ 2 files changed, 274 insertions(+), 2 deletions(-) create mode 100644 tests/test_signal_listing_range_reuse.py diff --git a/tests/fixtures/public_api_snapshot.json b/tests/fixtures/public_api_snapshot.json index c86c38b366..a3bf7bc9d3 100644 --- a/tests/fixtures/public_api_snapshot.json +++ b/tests/fixtures/public_api_snapshot.json @@ -1952,7 +1952,7 @@ "SignalAvailabilityType": "enums.signal_catalog_type", "SignalCatalogType": "enums.signal_catalog_type:SignalAvailabilityType", "SignalCoverageForecast": "core.signal_coverage_forecast", - "SignalCoverageRange": "signals.get_signals_response:Range", + "SignalCoverageRange": "core.signal_listing:Range", "SignalDefinitionEnrichment": "core.signal_definition_enrichment", "SignalFilters": "core.signal_filters", "SignalListing": "core.signal_listing", @@ -8846,7 +8846,7 @@ "Onboarder": "signals.get_signals_response", "ParentMatchBehavior": "signals.get_signals_response", "PreOnboardingPrecisionLevel": "signals.get_signals_response", - "Range": "signals.get_signals_response", + "Range": "core.signal_listing", "RefreshCadence": "signals.get_signals_response", "Right": "signals.get_signals_response", "Scope": "signals.get_signals_response", diff --git a/tests/test_signal_listing_range_reuse.py b/tests/test_signal_listing_range_reuse.py new file mode 100644 index 0000000000..65dee5ba02 --- /dev/null +++ b/tests/test_signal_listing_range_reuse.py @@ -0,0 +1,272 @@ +"""``fix_signal_listing_range_subclasses`` reuses ``SignalListing.Range`` by shape. + +A signal that extends ``core/signal-listing.json`` gets its own generated +``Range`` class, identical to ``SignalListing.Range`` but a different type. The +post-generation pass used to find that class by matching its generated text, +``extra='forbid'`` and all. The generator drifted (``StrictFloat``, new +descriptions), the text stopped matching, the duplicate classes stayed, and the +pass still printed success. adcp#8866 opens the schema (``extra='allow'``), +which would have kept the text match missing. + +The pass now locates each copy by name, compares it with ``SignalListing.Range`` +by what it validates (bases, ``model_config``, field types, constraints, +defaults; not descriptions), and raises when a copy diverges or survives. +""" + +from __future__ import annotations + +import ast +from pathlib import Path + +import pytest + +from scripts import post_generate_fixes as fixes + +FORBID = " model_config = ConfigDict(\n extra='forbid',\n )\n" +ALLOW = " model_config = ConfigDict(\n extra='allow',\n )\n" + +HEADER = "from __future__ import annotations\n\n" + +TARGETS = { + "signals/get_signals_response.py": ( + "from ..core.signal_listing import Range as _SignalListingRange\n" + ), + "core/wholesale_feed_event.py": "from .signal_listing import Range as _SignalListingRange\n", +} + + +def _range(config: str = FORBID, *, described: bool = False, body: str | None = None) -> str: + if body is None: + if described: + body = ( + " min: Annotated[StrictFloat, Field(description='Minimum value, inclusive.')]\n" + " max: Annotated[StrictFloat, Field(description='Maximum value, inclusive.')]\n" + ) + else: + body = " min: StrictFloat\n max: StrictFloat\n" + return f"class Range(AdCPBaseModel):\n{config}{body}" + + +def _listing(range_class: str | None) -> str: + text = ( + HEADER + + "from typing import Annotated\n\n" + + "from adcp.types.base import AdCPBaseModel, NoExplicitNull\n" + + "from pydantic import ConfigDict, Field, StrictFloat\n\n\n" + ) + if range_class: + text += range_class + "\n\n" + text += ( + "class SignalListing(AdCPBaseModel):\n" + " model_config = ConfigDict(\n extra='allow',\n )\n" + " range: Annotated[Range | None, NoExplicitNull] = None\n" + ) + return text + + +def _wholesale(range_class: str | None, import_line: str | None = None) -> str: + text = ( + HEADER + + "from typing import Annotated\n\n" + + "from adcp.types.base import AdCPBaseModel, NoExplicitNull\n" + + "from pydantic import ConfigDict, Field, StrictFloat\n\n" + + (import_line or "from .signal_listing import SignalListing\n") + + "\n\n" + ) + if range_class: + text += range_class + "\n\n" + text += "class Signal(SignalListing):\n" + text += " range: Annotated[Range | None, NoExplicitNull] = None\n" + return text + + +def _signals_response(range_class: str | None) -> str: + text = ( + HEADER + + "from typing import Annotated\n\n" + + "from adcp.types.base import AdCPBaseModel, NoExplicitNull\n" + + "from pydantic import ConfigDict, Field, StrictFloat\n\n" + + "from ..core.signal_listing import SignalListing\n" + + "from ..enums import signal_value_type\n\n\n" + ) + if range_class: + text += range_class + "\n\n" + text += "class Signal(AdCPBaseModel):\n" + text += " range: Annotated[Range | None, NoExplicitNull] = None\n" + return text + + +@pytest.fixture +def generated(tmp_path: Path, monkeypatch: pytest.MonkeyPatch) -> Path: + directory = tmp_path / "domains" + for sub in ("core", "signals"): + (directory / sub).mkdir(parents=True) + monkeypatch.setattr(fixes, "OUTPUT_DIR", directory) + return directory + + +def _write( + generated: Path, + listing: str, + signals: str | None, + wholesale: str | None, +) -> dict[str, Path]: + (generated / "core" / "signal_listing.py").write_text(listing) + paths = {name: generated / name for name in TARGETS} + if signals is not None: + paths["signals/get_signals_response.py"].write_text(signals) + if wholesale is not None: + paths["core/wholesale_feed_event.py"].write_text(wholesale) + return paths + + +def _assert_reused(paths: dict[str, Path]) -> None: + for name, alias_import in TARGETS.items(): + source = paths[name].read_text() + tree = ast.parse(source) + compile(source, name, "exec") + assert not any(isinstance(n, ast.ClassDef) and n.name == "Range" for n in tree.body), name + assert "Range = _SignalListingRange\n" in source, name + assert alias_import in source, name + + +def _scenario(generated: Path, config: str, *, listing_described: bool = True) -> dict[str, Path]: + return _write( + generated, + _listing(_range(config, described=listing_described)), + _signals_response(_range(config, described=True)), + _wholesale(_range(config)), + ) + + +def test_closed_copies_of_the_beta5_shape_are_reused(generated: Path) -> None: + paths = _scenario(generated, FORBID) + fixes.fix_signal_listing_range_subclasses() + _assert_reused(paths) + + +def test_open_copies_of_the_8866_shape_are_reused(generated: Path) -> None: + paths = _scenario(generated, ALLOW) + fixes.fix_signal_listing_range_subclasses() + _assert_reused(paths) + + +def test_copies_with_no_model_config_are_reused(generated: Path) -> None: + paths = _scenario(generated, "") + fixes.fix_signal_listing_range_subclasses() + _assert_reused(paths) + + +def test_description_only_differences_do_not_count(generated: Path) -> None: + listing = _range( + FORBID, + body=( + " min: Annotated[StrictFloat, Field(description='Lower bound.')]\n" + " max: Annotated[StrictFloat, Field(description='Upper bound.')]\n" + ), + ) + paths = _write( + generated, + _listing(listing), + _signals_response(_range(FORBID, described=True)), + _wholesale(_range(FORBID)), + ) + fixes.fix_signal_listing_range_subclasses() + _assert_reused(paths) + + +DIVERGENT = { + "listing open, copy closed": ( + _range(ALLOW, described=True), + _range(FORBID, described=True), + ), + "field type": ( + _range(FORBID, described=True), + _range(FORBID, body=" min: int\n max: StrictFloat\n"), + ), + "extra field": ( + _range(FORBID, described=True), + _range(FORBID, body=" min: StrictFloat\n max: StrictFloat\n step: StrictFloat\n"), + ), + "field constraint": ( + _range(FORBID, described=True), + _range( + FORBID, + body=( + " min: Annotated[StrictFloat, Field(ge=0)]\n" + " max: Annotated[StrictFloat, Field(description='Maximum value, inclusive.')]\n" + ), + ), + ), +} + + +@pytest.mark.parametrize("listing,copy", DIVERGENT.values(), ids=DIVERGENT.keys()) +@pytest.mark.parametrize("target", sorted(TARGETS)) +def test_a_divergent_copy_raises_and_leaves_the_file_alone( + generated: Path, listing: str, copy: str, target: str +) -> None: + signals = _signals_response(copy) + wholesale = _wholesale(copy) + paths = _write(generated, _listing(listing), signals, wholesale) + before = paths[target].read_text() + with pytest.raises(RuntimeError, match="no longer matches SignalListing.Range"): + fixes.fix_signal_listing_range_subclasses() + assert paths[target].read_text() == before + + +def test_running_twice_is_idempotent(generated: Path) -> None: + paths = _scenario(generated, FORBID) + fixes.fix_signal_listing_range_subclasses() + first = {name: path.read_text() for name, path in paths.items()} + fixes.fix_signal_listing_range_subclasses() + assert {name: path.read_text() for name, path in paths.items()} == first + + +def test_a_target_without_range_is_unchanged(generated: Path) -> None: + wholesale = _wholesale(None) + paths = _write(generated, _listing(_range(FORBID)), None, wholesale) + fixes.fix_signal_listing_range_subclasses() + assert paths["core/wholesale_feed_event.py"].read_text() == wholesale + assert not paths["signals/get_signals_response.py"].exists() + + +def test_a_target_range_without_a_listing_range_raises(generated: Path) -> None: + paths = _write(generated, _listing(None), None, _wholesale(_range(FORBID))) + before = paths["core/wholesale_feed_event.py"].read_text() + with pytest.raises(RuntimeError, match="core/signal_listing.py no longer does"): + fixes.fix_signal_listing_range_subclasses() + assert paths["core/wholesale_feed_event.py"].read_text() == before + + +def test_a_parenthesized_import_block_still_gets_the_alias_after_it(generated: Path) -> None: + wholesale = _wholesale( + _range(FORBID), + import_line="from .signal_listing import (\n SignalListing,\n)\n", + ) + paths = _write(generated, _listing(_range(FORBID)), None, wholesale) + fixes.fix_signal_listing_range_subclasses() + source = paths["core/wholesale_feed_event.py"].read_text() + compile(source, "wholesale_feed_event.py", "exec") + assert ( + "from .signal_listing import (\n SignalListing,\n)\n" + "from .signal_listing import Range as _SignalListingRange\n" + ) in source + assert "class Range" not in source + + +def test_the_generated_tree_shares_one_range_class() -> None: + from adcp.types.domains import signals + from adcp.types.domains.core import signal_listing, wholesale_feed_event + from adcp.types.domains.signals import get_signals_response + + assert get_signals_response.Range is signal_listing.Range + assert wholesale_feed_event.Range is signal_listing.Range + assert signals.Range is signal_listing.Range + assert ( + wholesale_feed_event.Signal.model_fields["range"].annotation + == signal_listing.SignalListing.model_fields["range"].annotation + ) + for module in (get_signals_response, wholesale_feed_event): + tree = ast.parse(Path(module.__file__).read_text()) + assert not any(isinstance(n, ast.ClassDef) and n.name == "Range" for n in tree.body) From cbb5fb7833031060b9c30a00c5014a6a361896f8 Mon Sep 17 00:00:00 2001 From: Brian O'Kelley Date: Mon, 9 Nov 2026 22:32:42 +0000 Subject: [PATCH 3/4] fix(codegen): reuse SignalListing.Range only where a module subclasses SignalListing The earlier commit aliased Range by assignment in two named files. That broke the domains invariant that a module binds only classes it defines (test_no_generated_type_is_reachable_under_zero_names). It also changed what signals/get_signals_response.Range is, though that module stopped subclassing SignalListing when 3.3 inlined its signal, so there is no parent type to match. The pass now scans every generated module for a SignalListing subclass that declares Range, deletes that copy when it validates like SignalListing.Range, and adds Range to the module's existing signal_listing import. It raises on a divergent copy, on an import it cannot extend, and on a surviving duplicate. get_signals_response keeps its own Range, so the public API snapshot and adcp.types.domains.signals.Range are unchanged. Co-Authored-By: Claude Opus 5.5 --- SCHEMA_DELTAS.md | 2 - docs/shared-type-names.md | 6 +- scripts/post_generate_fixes.py | 102 ++++++++++-------- .../domains/core/wholesale_feed_event.py | 8 +- .../domains/signals/get_signals_response.py | 10 +- tests/fixtures/public_api_snapshot.json | 4 +- 6 files changed, 74 insertions(+), 58 deletions(-) diff --git a/SCHEMA_DELTAS.md b/SCHEMA_DELTAS.md index 8a0f79ac0c..b76c65f799 100644 --- a/SCHEMA_DELTAS.md +++ b/SCHEMA_DELTAS.md @@ -4,5 +4,3 @@ - `core/wholesale_feed_event.py` - **classes removed**: Range -- `signals/get_signals_response.py` - - **classes removed**: Range diff --git a/docs/shared-type-names.md b/docs/shared-type-names.md index b4082b6aa9..f9d2a6875f 100644 --- a/docs/shared-type-names.md +++ b/docs/shared-type-names.md @@ -5,10 +5,10 @@ schemas legitimately declare a class called `QuerySummary` or `Creative`. `adcp.types` binds one of them per name. Import from the domain module to name the one you mean. -515 names, 1680 variants. +515 names, 1679 variants. Auto-generated by `scripts/consolidate_exports.py`. DO NOT EDIT MANUALLY. -Generation date: 2026-11-08 02:42:08 UTC +Generation date: 2026-11-09 22:26:05 UTC | Type name | Import one of | | --- | --- | @@ -371,7 +371,7 @@ Generation date: 2026-11-08 02:42:08 UTC | `Qualifier` | `adcp.types.domains.core.committed_metric.Qualifier`
`adcp.types.domains.core.delivery_metric_aggregate.Qualifier`
`adcp.types.domains.core.missing_metric.Qualifier`
`adcp.types.domains.core.package_delivery_metric_value.Qualifier`
`adcp.types.domains.core.performance_feedback.Qualifier`
`adcp.types.domains.core.performance_feedback_metric.Qualifier`
`adcp.types.domains.core.vendor_metric_value.Qualifier`
`adcp.types.domains.media_buy.package_forecast_input.Qualifier`
`adcp.types.domains.media_buy.package_request.Qualifier` | | `QuerySummary` | `adcp.types.domains.core.tasks_list_response.QuerySummary`
`adcp.types.domains.creative.list_creatives_response.QuerySummary`
`adcp.types.domains.protocol.list_tasks_response.QuerySummary` | | `Radius` | `adcp.types.domains.core.catchment.Radius`
`adcp.types.domains.core.product_filters.Radius`
`adcp.types.domains.core.product_offer_filters.Radius`
`adcp.types.domains.core.targeting.Radius`
`adcp.types.domains.core.targeting_input.Radius` | -| `Range` | `adcp.types.domains.core.audience_characteristic.Range`
`adcp.types.domains.core.signal_definition.Range`
`adcp.types.domains.core.signal_listing.Range`
`adcp.types.domains.core.wholesale_feed_event.Range`
`adcp.types.domains.property.property_feature_definition.Range`
`adcp.types.domains.protocol.get_adcp_capabilities_response.Range`
`adcp.types.domains.signals.get_signals_response.Range` | +| `Range` | `adcp.types.domains.core.audience_characteristic.Range`
`adcp.types.domains.core.signal_definition.Range`
`adcp.types.domains.core.signal_listing.Range`
`adcp.types.domains.property.property_feature_definition.Range`
`adcp.types.domains.protocol.get_adcp_capabilities_response.Range`
`adcp.types.domains.signals.get_signals_response.Range` | | `ReachWindow` | `adcp.types.domains.compliance.comply_test_controller_request.ReachWindow`
`adcp.types.domains.core.delivery_metrics.ReachWindow` | | `Reason` | `adcp.types.domains.adagents.Reason`
`adcp.types.domains.core.async_response_refs.creative.sync_creatives_async_response_input_required.Reason`
`adcp.types.domains.core.async_response_refs.media_buy.build_creative_async_response_input_required.Reason`
`adcp.types.domains.core.async_response_refs.media_buy.create_media_buy_async_response_input_required.Reason`
`adcp.types.domains.core.async_response_refs.media_buy.get_products_async_response_input_required.Reason`
`adcp.types.domains.core.async_response_refs.media_buy.sync_catalogs_async_response_input_required.Reason`
`adcp.types.domains.core.async_response_refs.media_buy.update_media_buy_async_response_input_required.Reason`
`adcp.types.domains.core.brand_response_authorization_result.Reason`
`adcp.types.domains.core.capabilities_changed_webhook.Reason`
`adcp.types.domains.core.catalog_item_availability_update.Reason`
`adcp.types.domains.core.principal_changed_webhook.Reason`
`adcp.types.domains.core.reporting_coverage.Reason`
`adcp.types.domains.core.reporting_subscription_state.Reason`
`adcp.types.domains.creative.sync_creatives_async_response_input_required.Reason`
`adcp.types.domains.error_details.agent_permission_denied.Reason`
`adcp.types.domains.error_details.execution_requirement_unmet.Reason`
`adcp.types.domains.media_buy.build_creative_async_response_input_required.Reason`
`adcp.types.domains.media_buy.create_media_buy_async_response_input_required.Reason`
`adcp.types.domains.media_buy.get_products_async_response_input_required.Reason`
`adcp.types.domains.media_buy.sync_catalogs_async_response_input_required.Reason`
`adcp.types.domains.media_buy.update_media_buy_async_response_input_required.Reason`
`adcp.types.domains.sponsored_intelligence.si_terminate_session_request.Reason` | | `ReasonCode` | `adcp.types.domains.core.account_status_changed_webhook.ReasonCode`
`adcp.types.domains.core.attestation_evaluation.ReasonCode`
`adcp.types.domains.core.reporting_adjustment.ReasonCode` | diff --git a/scripts/post_generate_fixes.py b/scripts/post_generate_fixes.py index 1ab60413c0..59c9dc28a6 100644 --- a/scripts/post_generate_fixes.py +++ b/scripts/post_generate_fixes.py @@ -5135,15 +5135,6 @@ def fix_mcp_webhook_operation_id_optional() -> None: print(" core/mcp_webhook_payload.py: made operation_id optional") -#: Generated modules whose signal redeclares ``range`` with its own copy of -#: ``SignalListing.Range``, keyed by path under ``OUTPUT_DIR``, with the module -#: each imports the shared class from. -_SIGNAL_LISTING_RANGE_REUSERS = { - ("signals", "get_signals_response.py"): "..core.signal_listing", - ("core", "wholesale_feed_event.py"): ".signal_listing", -} - - class _DropDescriptions(ast.NodeTransformer): """Strip ``description=`` from ``Field(...)`` and unwrap a bare ``Annotated[T]``.""" @@ -5199,22 +5190,36 @@ def _top_level_class(tree: ast.Module, name: str) -> ast.ClassDef | None: ) +def _extends_signal_listing(node: ast.ClassDef) -> bool: + return any( + (isinstance(base, ast.Name) and base.id == "SignalListing") + or (isinstance(base, ast.Attribute) and base.attr == "SignalListing") + for base in node.bases + ) + + def fix_signal_listing_range_subclasses() -> None: - """Reuse SignalListing.Range where a generated signal redeclares ``range``. - - A signal that extends ``core/signal-listing.json`` gets its own ``Range`` - class from the generator, identical to ``SignalListing.Range`` but a - different type, so a ``SignalListing.Range`` does not validate as the - subclass's ``range``. Each copy is found by name and compared with - ``SignalListing.Range`` by what it validates, so the pass keeps working when - the generator changes descriptions, scalar types or the schema's - ``additionalProperties``. It used to match the generated text, ``extra='forbid'`` - and all, and printed "not needed" once that text drifted. - - Raises when a copy no longer matches ``SignalListing.Range``, or when a - ``Range`` class would survive the pass: deleting a class that validates - differently changes the wire contract, and keeping a duplicate is the - silent miss this guards against. + """Reuse SignalListing.Range in every module that subclasses SignalListing. + + A class that extends ``core/signal-listing.json`` redeclares ``range``, and + the generator gives that redeclaration its own ``Range`` class: identical to + ``SignalListing.Range`` but a different type, so the subclass's ``range`` no + longer has its parent's type and a ``SignalListing.Range`` does not validate + there. Every generated module with a ``SignalListing`` subclass is checked. + Its ``Range`` is compared with ``SignalListing.Range`` by what it validates, + deleted, and imported from ``signal_listing`` instead, so the pass keeps + working when the generator changes descriptions, scalar types or the schema's + ``additionalProperties``. It used to match the generated text of two named + files, ``extra='forbid'`` and all, and reported success once that text drifted. + + A module that declares ``Range`` without subclassing ``SignalListing`` keeps + it, as every schema keeps its own classes: ``signals/get_signals_response.py`` + has inlined its signal since 3.3 and has no parent type to match. + + Raises when such a ``Range`` no longer validates like ``SignalListing.Range`` + (deleting it would change the wire contract), when the module imports + ``SignalListing`` in a form the shared ``Range`` cannot be added to, or when + a duplicate survives: a silent miss is a regeneration failure. """ listing_path = OUTPUT_DIR / "core" / "signal_listing.py" if not listing_path.exists(): @@ -5223,19 +5228,23 @@ class from the generator, identical to ``SignalListing.Range`` but a listing = _top_level_class(ast.parse(listing_path.read_text()), "Range") fixed = 0 - for parts, module in _SIGNAL_LISTING_RANGE_REUSERS.items(): - path = OUTPUT_DIR.joinpath(*parts) - relative = "/".join(parts) - if not path.exists(): - print(f" {relative} not found (skipping SignalListing.Range reuse)") + for path in sorted(OUTPUT_DIR.rglob("*.py")): + if path == listing_path: continue source = path.read_text() - local = _top_level_class(ast.parse(source), "Range") - if local is None: + if "SignalListing" not in source: + continue + tree = ast.parse(source) + local = _top_level_class(tree, "Range") + if local is None or not any( + isinstance(node, ast.ClassDef) and _extends_signal_listing(node) for node in tree.body + ): continue + relative = path.relative_to(OUTPUT_DIR).as_posix() if listing is None: raise RuntimeError( - f"{relative}: declares Range but core/signal_listing.py no longer does" + f"{relative}: subclasses SignalListing and declares Range, but " + "core/signal_listing.py no longer declares Range" ) if _range_shape(local) != _range_shape(listing): raise RuntimeError( @@ -5244,21 +5253,30 @@ class from the generator, identical to ``SignalListing.Range`` but a f" local: {_range_shape(local)!r}\n" f" listing: {_range_shape(listing)!r}" ) + imported = next( + ( + node + for node in tree.body + if isinstance(node, ast.ImportFrom) + and (node.module or "").split(".")[-1] == "signal_listing" + and any(a.name == "SignalListing" and a.asname is None for a in node.names) + ), + None, + ) + if imported is None: + raise RuntimeError( + f"{relative}: subclasses SignalListing without a " + "'from ...signal_listing import SignalListing' to add Range to" + ) - # Bind the shared class by assignment rather than import, so the module - # still declares ``Range``: ``scripts/consolidate_exports.py`` re-exports - # only declared names, and ``adcp.types.domains.signals.Range`` stays. lines = source.splitlines(keepends=True) assert local.end_lineno is not None start = sum(map(len, lines[: local.lineno - 1])) end = sum(map(len, lines[: local.end_lineno])) - source = source[:start] + "Range = _SignalListingRange\n" + source[end:] - imports = [n for n in ast.parse(source).body if isinstance(n, (ast.Import, ast.ImportFrom))] - if not imports or imports[-1].end_lineno is None: - raise RuntimeError(f"{relative}: no import block to add the Range import to") - lines = source.splitlines(keepends=True) - at = sum(map(len, lines[: imports[-1].end_lineno])) - source = source[:at] + f"from {module} import Range as _SignalListingRange\n" + source[at:] + if source[start - 2 : start] == "\n\n": + start -= 2 + source = source[:start] + source[end:] + source = add_to_import(source, "." * imported.level + (imported.module or ""), "Range") if _top_level_class(ast.parse(source), "Range") is not None: raise RuntimeError(f"{relative}: a duplicate Range class survived the reuse pass") diff --git a/src/adcp/types/domains/core/wholesale_feed_event.py b/src/adcp/types/domains/core/wholesale_feed_event.py index b33e333a5b..9858e8ad27 100644 --- a/src/adcp/types/domains/core/wholesale_feed_event.py +++ b/src/adcp/types/domains/core/wholesale_feed_event.py @@ -1,6 +1,6 @@ # generated by datamodel-codegen: # filename: core/wholesale_feed_event.json -# timestamp: 2026-11-09T21:22:22+00:00 +# timestamp: 2026-11-09T22:24:53+00:00 from __future__ import annotations @@ -20,8 +20,7 @@ from . import signal_id as signal_id_1 from . import signal_ref as signal_ref_1 from . import vendor_pricing_option -from .signal_listing import SignalListing -from .signal_listing import Range as _SignalListingRange +from .signal_listing import Range, SignalListing class EventType(StrEnum): @@ -91,9 +90,6 @@ class RemovalReason(StrEnum): policy_takedown = 'policy_takedown' -Range = _SignalListingRange - - class Payload(AdCPBaseModel): model_config = ConfigDict( extra='allow', diff --git a/src/adcp/types/domains/signals/get_signals_response.py b/src/adcp/types/domains/signals/get_signals_response.py index 124e75f29c..92984db949 100644 --- a/src/adcp/types/domains/signals/get_signals_response.py +++ b/src/adcp/types/domains/signals/get_signals_response.py @@ -1,6 +1,6 @@ # generated by datamodel-codegen: # filename: signals/get_signals_response.json -# timestamp: 2026-11-09T21:22:22+00:00 +# timestamp: 2026-10-31T11:55:18+00:00 from __future__ import annotations @@ -29,10 +29,14 @@ from ..enums import restricted_attribute, signal_catalog_type from ..enums import signal_status as signal_status_1 from ..enums import signal_value_type -from ..core.signal_listing import Range as _SignalListingRange -Range = _SignalListingRange +class Range(AdCPBaseModel): + model_config = ConfigDict( + extra='forbid', + ) + min: Annotated[StrictFloat, Field(description='Minimum value, inclusive.')] + max: Annotated[StrictFloat, Field(description='Maximum value, inclusive.')] class Value(AdCPBaseModel): diff --git a/tests/fixtures/public_api_snapshot.json b/tests/fixtures/public_api_snapshot.json index a3bf7bc9d3..c86c38b366 100644 --- a/tests/fixtures/public_api_snapshot.json +++ b/tests/fixtures/public_api_snapshot.json @@ -1952,7 +1952,7 @@ "SignalAvailabilityType": "enums.signal_catalog_type", "SignalCatalogType": "enums.signal_catalog_type:SignalAvailabilityType", "SignalCoverageForecast": "core.signal_coverage_forecast", - "SignalCoverageRange": "core.signal_listing:Range", + "SignalCoverageRange": "signals.get_signals_response:Range", "SignalDefinitionEnrichment": "core.signal_definition_enrichment", "SignalFilters": "core.signal_filters", "SignalListing": "core.signal_listing", @@ -8846,7 +8846,7 @@ "Onboarder": "signals.get_signals_response", "ParentMatchBehavior": "signals.get_signals_response", "PreOnboardingPrecisionLevel": "signals.get_signals_response", - "Range": "core.signal_listing", + "Range": "signals.get_signals_response", "RefreshCadence": "signals.get_signals_response", "Right": "signals.get_signals_response", "Scope": "signals.get_signals_response", From b44fe3287f5624768d5eaad27355ebfc4cd709aa Mon Sep 17 00:00:00 2001 From: Brian O'Kelley Date: Mon, 9 Nov 2026 22:36:15 +0000 Subject: [PATCH 4/4] test(codegen): cover the SignalListing-subclass scan for the Range reuse Co-Authored-By: Claude Opus 5.5 --- tests/test_signal_listing_range_reuse.py | 221 +++++++++++++---------- 1 file changed, 126 insertions(+), 95 deletions(-) diff --git a/tests/test_signal_listing_range_reuse.py b/tests/test_signal_listing_range_reuse.py index 65dee5ba02..9a83058016 100644 --- a/tests/test_signal_listing_range_reuse.py +++ b/tests/test_signal_listing_range_reuse.py @@ -1,16 +1,21 @@ """``fix_signal_listing_range_subclasses`` reuses ``SignalListing.Range`` by shape. -A signal that extends ``core/signal-listing.json`` gets its own generated +A class that extends ``core/signal-listing.json`` gets its own generated ``Range`` class, identical to ``SignalListing.Range`` but a different type. The -post-generation pass used to find that class by matching its generated text, -``extra='forbid'`` and all. The generator drifted (``StrictFloat``, new -descriptions), the text stopped matching, the duplicate classes stayed, and the -pass still printed success. adcp#8866 opens the schema (``extra='allow'``), -which would have kept the text match missing. - -The pass now locates each copy by name, compares it with ``SignalListing.Range`` -by what it validates (bases, ``model_config``, field types, constraints, -defaults; not descriptions), and raises when a copy diverges or survives. +post-generation pass used to find that class by matching the generated text of +two named files, ``extra='forbid'`` and all. The generator drifted +(``StrictFloat``, new descriptions), the text stopped matching, the duplicate +classes stayed, and the pass still printed success. adcp#8866 opens the schema +(``extra='allow'``), which would have kept the text match missing. + +The pass now scans every generated module for a ``SignalListing`` subclass that +declares ``Range``, compares the copy with ``SignalListing.Range`` by what it +validates (bases, ``model_config``, field types, constraints, defaults; not +descriptions), deletes it, and adds ``Range`` to the module's +``signal_listing`` import. It raises when a copy diverges, when the import +cannot be extended, or when a ``Range`` survives. A module that declares +``Range`` without subclassing ``SignalListing`` (``get_signals_response`` +inlines its signal) keeps its own. """ from __future__ import annotations @@ -27,12 +32,17 @@ HEADER = "from __future__ import annotations\n\n" -TARGETS = { - "signals/get_signals_response.py": ( - "from ..core.signal_listing import Range as _SignalListingRange\n" - ), - "core/wholesale_feed_event.py": "from .signal_listing import Range as _SignalListingRange\n", +# Subclass modules in two domains, to show the scan is not tied to file names. +SUBCLASSES = { + "core/wholesale_feed_event.py": "from .signal_listing import SignalListing\n", + "media_buy/some_feed.py": "from ..core.signal_listing import SignalListing\n", } +INLINE = "signals/get_signals_response.py" +IMPORTS = ( + "from typing import Annotated\n\n" + "from adcp.types.base import AdCPBaseModel, NoExplicitNull\n" + "from pydantic import ConfigDict, Field, StrictFloat\n\n" +) def _range(config: str = FORBID, *, described: bool = False, body: str | None = None) -> str: @@ -48,12 +58,7 @@ def _range(config: str = FORBID, *, described: bool = False, body: str | None = def _listing(range_class: str | None) -> str: - text = ( - HEADER - + "from typing import Annotated\n\n" - + "from adcp.types.base import AdCPBaseModel, NoExplicitNull\n" - + "from pydantic import ConfigDict, Field, StrictFloat\n\n\n" - ) + text = HEADER + IMPORTS + "\n" if range_class: text += range_class + "\n\n" text += ( @@ -64,78 +69,69 @@ def _listing(range_class: str | None) -> str: return text -def _wholesale(range_class: str | None, import_line: str | None = None) -> str: - text = ( - HEADER - + "from typing import Annotated\n\n" - + "from adcp.types.base import AdCPBaseModel, NoExplicitNull\n" - + "from pydantic import ConfigDict, Field, StrictFloat\n\n" - + (import_line or "from .signal_listing import SignalListing\n") - + "\n\n" - ) +def _subclass( + range_class: str | None, + import_line: str = "from .signal_listing import SignalListing\n", + base: str = "SignalListing", +) -> str: + text = HEADER + IMPORTS + import_line + "\n\n" if range_class: text += range_class + "\n\n" - text += "class Signal(SignalListing):\n" - text += " range: Annotated[Range | None, NoExplicitNull] = None\n" + text += f"class Signal({base}):\n range: Annotated[Range | None, NoExplicitNull] = None\n" return text -def _signals_response(range_class: str | None) -> str: - text = ( +def _inline(range_class: str) -> str: + return ( HEADER - + "from typing import Annotated\n\n" - + "from adcp.types.base import AdCPBaseModel, NoExplicitNull\n" - + "from pydantic import ConfigDict, Field, StrictFloat\n\n" - + "from ..core.signal_listing import SignalListing\n" - + "from ..enums import signal_value_type\n\n\n" + + IMPORTS + + "from ..core.signal_listing import SignalListing\n\n\n" + + range_class + + "\n\n" + + "class Signal(AdCPBaseModel):\n" + + " range: Annotated[Range | None, NoExplicitNull] = None\n" ) - if range_class: - text += range_class + "\n\n" - text += "class Signal(AdCPBaseModel):\n" - text += " range: Annotated[Range | None, NoExplicitNull] = None\n" - return text @pytest.fixture def generated(tmp_path: Path, monkeypatch: pytest.MonkeyPatch) -> Path: directory = tmp_path / "domains" - for sub in ("core", "signals"): + for sub in ("core", "signals", "media_buy"): (directory / sub).mkdir(parents=True) monkeypatch.setattr(fixes, "OUTPUT_DIR", directory) return directory -def _write( - generated: Path, - listing: str, - signals: str | None, - wholesale: str | None, -) -> dict[str, Path]: +def _put(generated: Path, listing: str, modules: dict[str, str]) -> dict[str, Path]: (generated / "core" / "signal_listing.py").write_text(listing) - paths = {name: generated / name for name in TARGETS} - if signals is not None: - paths["signals/get_signals_response.py"].write_text(signals) - if wholesale is not None: - paths["core/wholesale_feed_event.py"].write_text(wholesale) + paths = {} + for name, text in modules.items(): + paths[name] = generated / name + paths[name].write_text(text) return paths def _assert_reused(paths: dict[str, Path]) -> None: - for name, alias_import in TARGETS.items(): - source = paths[name].read_text() + for name, path in paths.items(): + source = path.read_text() tree = ast.parse(source) compile(source, name, "exec") assert not any(isinstance(n, ast.ClassDef) and n.name == "Range" for n in tree.body), name - assert "Range = _SignalListingRange\n" in source, name - assert alias_import in source, name + assert "import Range, SignalListing\n" in source, name + assert "_SignalListingRange" not in source, name -def _scenario(generated: Path, config: str, *, listing_described: bool = True) -> dict[str, Path]: - return _write( +def _scenario(generated: Path, config: str) -> dict[str, Path]: + return _put( generated, - _listing(_range(config, described=listing_described)), - _signals_response(_range(config, described=True)), - _wholesale(_range(config)), + _listing(_range(config, described=True)), + { + "core/wholesale_feed_event.py": _subclass(_range(config)), + "media_buy/some_feed.py": _subclass( + _range(config, described=True), + "from ..core.signal_listing import SignalListing\n", + ), + }, ) @@ -165,11 +161,10 @@ def test_description_only_differences_do_not_count(generated: Path) -> None: " max: Annotated[StrictFloat, Field(description='Upper bound.')]\n" ), ) - paths = _write( + paths = _put( generated, _listing(listing), - _signals_response(_range(FORBID, described=True)), - _wholesale(_range(FORBID)), + {"core/wholesale_feed_event.py": _subclass(_range(FORBID))}, ) fixes.fix_signal_listing_range_subclasses() _assert_reused(paths) @@ -202,19 +197,31 @@ def test_description_only_differences_do_not_count(generated: Path) -> None: @pytest.mark.parametrize("listing,copy", DIVERGENT.values(), ids=DIVERGENT.keys()) -@pytest.mark.parametrize("target", sorted(TARGETS)) +@pytest.mark.parametrize("target", sorted(SUBCLASSES)) def test_a_divergent_copy_raises_and_leaves_the_file_alone( generated: Path, listing: str, copy: str, target: str ) -> None: - signals = _signals_response(copy) - wholesale = _wholesale(copy) - paths = _write(generated, _listing(listing), signals, wholesale) + paths = _put( + generated, + _listing(listing), + {target: _subclass(copy, SUBCLASSES[target])}, + ) before = paths[target].read_text() with pytest.raises(RuntimeError, match="no longer matches SignalListing.Range"): fixes.fix_signal_listing_range_subclasses() assert paths[target].read_text() == before +def test_a_module_that_does_not_subclass_keeps_its_own_range(generated: Path) -> None: + # get_signals_response inlines its signal: no parent type, so no reuse and no raise, + # even when its Range differs from SignalListing.Range. + for copy in (_range(FORBID, described=True), _range(ALLOW, body=" min: int\n")): + inline = _inline(copy) + paths = _put(generated, _listing(_range(FORBID, described=True)), {INLINE: inline}) + fixes.fix_signal_listing_range_subclasses() + assert paths[INLINE].read_text() == inline + + def test_running_twice_is_idempotent(generated: Path) -> None: paths = _scenario(generated, FORBID) fixes.fix_signal_listing_range_subclasses() @@ -223,50 +230,74 @@ def test_running_twice_is_idempotent(generated: Path) -> None: assert {name: path.read_text() for name, path in paths.items()} == first -def test_a_target_without_range_is_unchanged(generated: Path) -> None: - wholesale = _wholesale(None) - paths = _write(generated, _listing(_range(FORBID)), None, wholesale) +def test_a_subclass_without_range_is_unchanged(generated: Path) -> None: + text = _subclass(None) + paths = _put(generated, _listing(_range(FORBID)), {"core/wholesale_feed_event.py": text}) fixes.fix_signal_listing_range_subclasses() - assert paths["core/wholesale_feed_event.py"].read_text() == wholesale - assert not paths["signals/get_signals_response.py"].exists() + assert paths["core/wholesale_feed_event.py"].read_text() == text -def test_a_target_range_without_a_listing_range_raises(generated: Path) -> None: - paths = _write(generated, _listing(None), None, _wholesale(_range(FORBID))) +def test_a_subclass_range_without_a_listing_range_raises(generated: Path) -> None: + paths = _put( + generated, + _listing(None), + {"core/wholesale_feed_event.py": _subclass(_range(FORBID))}, + ) before = paths["core/wholesale_feed_event.py"].read_text() - with pytest.raises(RuntimeError, match="core/signal_listing.py no longer does"): + with pytest.raises(RuntimeError, match="core/signal_listing.py no longer declares Range"): fixes.fix_signal_listing_range_subclasses() assert paths["core/wholesale_feed_event.py"].read_text() == before -def test_a_parenthesized_import_block_still_gets_the_alias_after_it(generated: Path) -> None: - wholesale = _wholesale( - _range(FORBID), - import_line="from .signal_listing import (\n SignalListing,\n)\n", +def test_a_parenthesized_import_gets_range_inside_it(generated: Path) -> None: + paths = _put( + generated, + _listing(_range(FORBID)), + { + "core/wholesale_feed_event.py": _subclass( + _range(FORBID), + import_line="from .signal_listing import (\n SignalListing,\n)\n", + ) + }, ) - paths = _write(generated, _listing(_range(FORBID)), None, wholesale) fixes.fix_signal_listing_range_subclasses() source = paths["core/wholesale_feed_event.py"].read_text() compile(source, "wholesale_feed_event.py", "exec") - assert ( - "from .signal_listing import (\n SignalListing,\n)\n" - "from .signal_listing import Range as _SignalListingRange\n" - ) in source + assert "from .signal_listing import (\n Range,\n SignalListing,\n)\n" in source assert "class Range" not in source -def test_the_generated_tree_shares_one_range_class() -> None: +def test_an_attribute_base_without_a_direct_import_raises(generated: Path) -> None: + text = _subclass( + _range(FORBID), + import_line="from . import signal_listing as signal_listing_1\n", + base="signal_listing_1.SignalListing", + ) + paths = _put(generated, _listing(_range(FORBID)), {"core/wholesale_feed_event.py": text}) + with pytest.raises( + RuntimeError, + match="subclasses SignalListing without a 'from ...signal_listing import SignalListing'", + ): + fixes.fix_signal_listing_range_subclasses() + assert paths["core/wholesale_feed_event.py"].read_text() == text + + +def test_the_generated_tree_shares_range_only_where_it_subclasses() -> None: from adcp.types.domains import signals from adcp.types.domains.core import signal_listing, wholesale_feed_event from adcp.types.domains.signals import get_signals_response - assert get_signals_response.Range is signal_listing.Range assert wholesale_feed_event.Range is signal_listing.Range - assert signals.Range is signal_listing.Range assert ( wholesale_feed_event.Signal.model_fields["range"].annotation == signal_listing.SignalListing.model_fields["range"].annotation ) - for module in (get_signals_response, wholesale_feed_event): - tree = ast.parse(Path(module.__file__).read_text()) - assert not any(isinstance(n, ast.ClassDef) and n.name == "Range" for n in tree.body) + assert get_signals_response.Range is not signal_listing.Range + assert signals.Range is get_signals_response.Range + + def declares_range(module: object) -> bool: + tree = ast.parse(Path(module.__file__).read_text()) # type: ignore[attr-defined] + return any(isinstance(n, ast.ClassDef) and n.name == "Range" for n in tree.body) + + assert not declares_range(wholesale_feed_event) + assert declares_range(get_signals_response)