fix(types): add UpdateMediaBuyResponse1 ergonomic coercion#864
Merged
Conversation
There was a problem hiding this comment.
LGTM. Follow-ups noted below. Right call to mirror the CreateMediaBuyResponse1 coercion shape on the update-side success arm — the _find_media_buy_success_variant(module) refactor generalizes cleanly and _ergonomic.py:505-528 matches the upstream field declarations exactly (Sequence[Package] for affected_packages, list[Package] for packages, MediaBuyStatus for media_buy_status).
Things I checked
- Patch annotations mirror the generated source:
update_media_buy_response.py:31-34declaresaffected_packages: Sequence[Package] | None,packages: list[Package] | None,media_buy_status: MediaBuyStatus | None—_ergonomic.py:509-528preserves theSequencevslistdistinction, which is load-bearing per the file preamble. _find_media_buy_success_variantcannot false-positive: only*Response1in either module carriesmedia_buy_id(verified across bothcreate_media_buy_response.pyandupdate_media_buy_response.py).- Container detection in the comment generator (
get_origin(base_ann) is AbcSequence) trackspost_generate_fixes.rewrite_response_list_to_sequence/widen_extension_point_lists_to_sequenceoutputs — the fourlist[X] → Sequence[X]comment churn sites atPackageRequest.creatives,CreateMediaBuyRequest.packages,ListCreativesResponse.creatives,GetMediaBuyDeliveryResponse.media_buy_deliveriesare correctness, not cosmetic. - Legacy
status → media_buy_statusbridging (media_buy_status_helpers.MEDIA_BUY_LEGACY_STATUS_VALUES) is preserved: the@model_validator(mode='before')atupdate_media_buy_response.py:37-57runs ahead of the new field-levelBeforeValidator, so the normalizer fires first. - Conventional-commit prefix is right:
fix(types):is non-breaking and additive. No public-API removal. - Public surface unchanged:
UpdateMediaBuyResponse1/UpdateMediaBuySuccessResponsewere already exported (__init__.py:613,616,aliases.py:214,607); this PR only loosens input validation, no JSON-schema export change.
Follow-ups (non-blocking — file as issues)
UpdateMediaBuyResponse1.packagespatches a codegen artifact, not a wire field. Perad-tech-protocol-expert: the 3.1.0-beta.3, 3.0, and 2.5 update-media-buy-response schemas define onlyaffected_packageson the success arm —packagesatupdate_media_buy_response.py:32has no upstream basis. The patch is harmless (extra='allow'already accepts it, no wire round-trip change) but advertises a field the spec doesn't define. Either fix the codegen to drop the spuriouspackages, or drop the patch + the corresponding test assertion. Cross-refs: adcontextprotocol/adcp#4906, #4895.- Document the
exclude=Truediscipline for subclass-list coercion.coerce_subclass_listwidens input variance, butmodel_config = ConfigDict(extra='allow')on the response means subclass fields withoutField(..., exclude=True)will leak intomodel_dump(). The new tests use it correctly; the docstring oncoerce_subclass_listcould note it. Same risk already exists onCreateMediaBuyResponse1.packages— not introduced here.
Minor nits (non-blocking)
- Sequence coverage in the new test.
tests/test_type_coercion.py:454-468exercisesaffected_packageswith a plainlist, which already worked under the oldlist[Package]annotation. Passing a tuple (or any non-listSequence) would actually prove the covariant-Sequenceintent of the patch. - Hoist
_find_media_buy_success_variant. It's a closure insidegenerate_code()(scripts/generate_ergonomic_coercion.py:267). Promoting it to module scope alongsideget_base_typewould make it independently testable. Cosmetic.
Approving on the strength of the protocol-shape check plus the Sequence/list parity with the generated source.
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
Replaces #857 on a repo-owned branch so required base-repo checks can run normally.
Follow-up to #854.
This extends the generated ergonomic coercion pass to include the update-media-buy success response arm.
UpdateMediaBuySuccessResponse/UpdateMediaBuyResponse1now gets the same package-subclass andmedia_buy_statusstring coercion treatment thatCreateMediaBuyResponse1already had.What changed
UpdateMediaBuyResponse1toscripts/generate_ergonomic_coercion.pyresponse analysis.src/adcp/types/_ergonomic.pysoaffected_packages,packages, andmedia_buy_statusare patched at import time.media_buy_statuscoercion.Local validation
PYTHONPATH=src pytest tests/test_type_coercion.py tests/test_code_generation.py -q --tb=shortPYTHONPATH=src pytest tests/test_type_aliases.py tests/test_public_api.py tests/type_checks -q --tb=shortPYTHONPATH=src ruff check scripts/generate_ergonomic_coercion.py src/adcp/types/_ergonomic.py tests/test_type_coercion.py