chore(canonical-formats): polish bundle (B+C+D+E+F)#849
Merged
Conversation
…ixtures module, group helper, version fwd-compat Bundles the deferred follow-ups from #845's expert review. Five small changes; no behavioral surprise. ## B — projection.py → v2_to_v1.py Symmetric with v1_to_v2.py. ``git mv`` preserves history. Imports continue to work via the package re-export. Adopters who reached into the private path ``from adcp.canonical_formats.projection`` must switch to ``from adcp.canonical_formats.v2_to_v1``. ## C — Typed Divergence dataclass ``check_narrows`` returns ``list[Divergence]`` instead of ``list[dict[str, Any]]``. ``Divergence.to_dict()`` preserves the original advisory ``details.divergences`` key vocabulary (``v1_max`` / ``v1_min`` / ``v1_allowed`` / ``v1_value``) so wire parsers aren't affected. Adopters calling ``check_narrows`` and indexing ``d["field"]`` must switch to ``d.field``. ## D — adcp.canonical_formats.fixtures public module 14 v2 + 50 v1 fixtures bundled under ``src/adcp/canonical_formats/_fixtures/`` and exposed via ``load_reference_product(name)``, ``load_v1_reference_catalog()``, ``REFERENCE_PRODUCT_NAMES``. Adopters reuse without re-vendoring. ## E — group_declarations_by_product After ``project_v1_catalog_to_v2``, buckets declarations into per- product ``format_options[]`` lists given a ``{product_id: [v1_format_id, ...]}`` mapping. Bucket key is the first ``v1_format_ref`` (matches ``find_declaration_by_v1_format_id`` semantics). ## F — _versions_overlap forward-compat Unknown DSL operator prefixes (``~>``, ``^``) log WARNING and treat as non-matching, rather than raising. Registry MAY publish operators ahead of SDK support; crashing cached sessions is worse than missing a match. ## Tests 5271 pass. New ``test_canonical_formats_fixtures.py`` covers the public loader. ``test_canonical_formats_v1_to_v2.py`` extended for ``group_declarations_by_product``. Narrowing + registry tests updated for typed Divergence and forward-compat tone. Refs: #741, #845
There was a problem hiding this comment.
LGTM. Follow-ups noted below. Wire-shape vocabulary is preserved character-identical against #845, the polish surface is narrow, and the bundle is well-tested.
Things I checked
Divergence.to_dict()atsrc/adcp/canonical_formats/narrowing.py:90-109emits the exactdetails.divergenceskey set (v1_max/v1_min/v1_allowed/v1_valuepaired withv2_value/v2_declared) that #845 already shipped on the wire. The four-arm closedDivergenceKindliteral matches the fourkindstrings the half-2 advisory was already emitting. Adopters indexing the advisory dict are not affected; only adopters who calledcheck_narrowsdirectly need to switch to attribute access.- 14 v2 product fixtures conform to
schemas/cache/3.1.0-beta.3/core/product.json. Spot-checkedchatgpt_brand_mention(agent_placement+sponsored_intelligence),google_performance_max(responsive_creative),meta_carousel(image_carousel) — allformat_kindvalues are in the publishedcanonical-format-kind.jsonenum, all carry validpricing_options[].pricing_model,delivery_type, andreporting_capabilities. _versions_overlaplog-and-skip on unknown operator prefixes is the right protocol posture — it mirrors the open-on-consumer pattern already established bycanonical-format-kind.json's normative "Consumer SDKs MUST treat this enum as open at parse time."group_declarations_by_productfirst-ref grouping is sound for the documented input contract:project_v1_format_to_declarationconstructs declarations with single-entryv1_format_refatv1_to_v2.py:219-223, so the "multi-size fan-out" docstring caveat is forward-looking only.pyproject.tomlpackage-data inclusion ofcanonical_formats/_fixtures/*.jsonis wired correctly so the fixtures ship in the wheel.projection.py→v2_to_v1.pyis a puregit mv; no in-repo importer reaches into the old private path.- Test plan checked: 5271 pass, 11 new tests, wire-shape preservation pinned at
tests/test_canonical_formats_narrowing.py:64-69, 115-116.
Follow-ups (non-blocking — file as issues)
.xvs operator-prefix asymmetry in_versions_overlap.src/adcp/canonical_formats/registry.py:213-220still raisesValueErroron an unparseable.xmajor, while the operator-prefix branch at:234-242log-and-skips. The forward-compat doc at:195-203reads as if the whole DSL log-and-skips. A registry-side"foo.x"typo crashes the matcher; a"~>foo"typo does not. Either flip the.xbranch to symmetric log+continueor sharpen the docstring to call the.xmalformed path out as intentionally strict. (code-reviewerflag.)_UnknownFixtureErroris underscore-prefixed but raised by the publicload_reference_product.src/adcp/canonical_formats/fixtures.py:55-58. Adopters can only catch via theValueErrorparent, which defeats the typed-exception value. Either rename toUnknownFixtureErrorand add to__all__, or document that the contract is catchingValueError.group_declarations_by_productcollision semantics are documented but untested.src/adcp/canonical_formats/v1_to_v2.py:470-473— the first-product-winssetdefaultis the right call, buttests/test_canonical_formats_v1_to_v2.py:222-305never pins it. Add a collision test so future refactors can't silently flip the semantic.- Commit semver signal.
chore(canonical-formats):for a PR that adds three public exports (Divergence,DivergenceKind,group_declarations_by_product) and flips the return type ofcheck_narrowsis the wrong release-please signal — strict reading isfeat!:with aBREAKING CHANGE:footer. The mitigation is real (namespace landed days ago in the same 6.1 beta cycle, documented Experimental,MIGRATION_v5_to_v6.mdis updated to enumerate the breaking surface) which is why this isn't blocking, but a notable choice for the next polish bundle to get right.
Minor nits (non-blocking)
group_declarations_by_productdocstring forward-looking caveat.src/adcp/canonical_formats/v1_to_v2.py:480-483explains therefs[0]choice in terms of "multi-size fan-out," but the catalog-projection codepath produces single-ref declarations by construction. One-line precondition note ("input declarations fromproject_v1_catalog_to_v2carry exactly one ref") would prevent a future adopter from passing in a hand-authored multi-ref and silently losing product membership for non-first refs.
Approved.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Bundles the five deferred follow-ups from #845's expert review. No behavioral surprise — additive public API, internal rename with re-exports, and one fail-loud → log-and-skip flip.
What's in the bundle
projection.py→v2_to_v1.pyfor symmetry withv1_to_v2.py.git mvpreserves history; package re-exports unchanged.Divergencedataclass (field/kind/cap/value+DivergenceKindLiteral).check_narrowsreturnslist[Divergence].Divergence.to_dict()preserves the wire-shapedetails.divergenceskey vocabulary.adcp.canonical_formats.fixturespublic module — adopters reuse the 14 v2 + 50 v1 vendored fixtures viaload_reference_product(name)/load_v1_reference_catalog()/REFERENCE_PRODUCT_NAMESwithout re-vendoring upstream.group_declarations_by_product(decls, mapping)— buckets the output ofproject_v1_catalog_to_v2into per-productformat_options[]lists._versions_overlaplog-and-skip on unknown DSL operator prefixes (~>,^) instead of raise. Registry-side forward-compat posture.Public-API additions
3 new names on
adcp.canonical_formats:Divergence(dataclass)DivergenceKind(Literal)group_declarations_by_product(function)Plus the new submodule
adcp.canonical_formats.fixtures(reached in explicitly, not auto-re-exported on the package surface).Breaking surface (very narrow)
check_narrowsreturn type:list[dict[str, Any]]→list[Divergence]. The wire-shape projectionDivergence.to_dict()preserves the original key vocabulary; only adopters who calledcheck_narrowsand indexed the raw dict need to switch to attribute access._versions_overlapno longer raisesValueErroron unknown operators — it logs and returnsFalse. This was internal but documented as a public matcher in the registry module.Adopters who reached into the private path
from adcp.canonical_formats.projection import ...must switch tofrom adcp.canonical_formats.v2_to_v1 import .... The documented public path (from adcp.canonical_formats import project_product_to_v1) is unchanged.Test plan
ruff check src/— passedmypy src/adcp/— passed (901 source files)pytest tests/— 5271 passed, 38 skipped, 1 xfailedPolish backlog: complete
After this lands, the #741 polish backlog from PR #847's description is fully addressed.
Refs: #741, #845