From e90568423d49508c9a730cd58f890cc068a1f610 Mon Sep 17 00:00:00 2001 From: Brian O'Kelley Date: Tue, 6 Oct 2026 13:43:37 +0000 Subject: [PATCH 1/2] fix(migrate): rewrite generated_poc imports to adcp.types.domains, split-aware MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The `generated_poc` DeprecationWarning and docs/types-9-migration.md told adopters that `adcp migrate v3-to-v4` rewrites the old import path. It did not: the codemod only flagged those lines, with a stale "import from adcp.types instead" message, even under --auto-apply. The codemod now applies the rename. Every `adcp.types.generated_poc` reference — a `from` import, a module import, a dotted attribute access, the module half of a multi-line import — becomes `adcp.types.domains` under the same stem, reported as a `rename_import` finding and rewritten by --apply; the generated tree moved there with its stems unchanged and the deprecated path returns the same objects, so the rewrite is mechanical. The one stem that split, `brand`, is resolved per imported name against the two halves it became (`domains.brand_discovery` for brand.json's classes, discovery half first as the runtime alias serves it, `domains.brand` for the task-schema aggregator); a line that mixes the halves becomes two import statements, and a bare `generated_poc.brand` module reference or a name neither half binds is flagged with both halves named. --auto-apply still lifts a line to `adcp.types` when every name on it is a known public alias there, and now prefix-renames the rest instead of leaving a mixed line on the deprecated path. The split halves are discovered from the installed SDK with the generator's `_discovery` suffix, which a test pins equal to the runtime alias module's; the codemod cannot import that private module under the import-layering rule. The warning text, the CLI help, MIGRATION_v3_to_v4.md and the migration note now name `adcp migrate v3-to-v4 --apply`, and `adcp.types`'s module docstring no longer forbids `adcp.types.domains`, which the warning and docs point adopters to for a name the flat namespace cannot bind. Closes #1417 Co-Authored-By: Claude Fable 5.1 --- MIGRATION_v3_to_v4.md | 36 +- docs/types-9-migration.md | 7 +- src/adcp/migrate/__main__.py | 5 +- src/adcp/migrate/v3_to_v4.py | 487 +++++++++++++++++-------- src/adcp/types/__init__.py | 16 +- src/adcp/types/_generated_poc_alias.py | 12 +- tests/test_migrate_v3_to_v4.py | 380 +++++++++++++++---- 7 files changed, 699 insertions(+), 244 deletions(-) diff --git a/MIGRATION_v3_to_v4.md b/MIGRATION_v3_to_v4.md index 7e22b7e60..c0ca6f4ec 100644 --- a/MIGRATION_v3_to_v4.md +++ b/MIGRATION_v3_to_v4.md @@ -25,11 +25,16 @@ python -m adcp.migrate v3-to-v4 ./src --auto-apply python -m adcp.migrate v3-to-v4 ./src --json ``` -`--auto-apply` implies `--apply` and additionally rewrites the ~78% of findings -that are mechanically safe: `from adcp.types.generated_poc.X import Symbol` lines -where every symbol has a known public alias on `adcp.types`, and `Assets` numbered -classes that have a documented semantic alias (`Assets81 → VideoFormatAsset`, etc.). -`flag_removed` findings always require human attention. +`--apply` rewrites the nine `Asset` renames and every +`adcp.types.generated_poc` import path to `adcp.types.domains`, where the +generated tree lives from 9.0 under the same module stems (see +[generated_poc brand split](#generated_poc-brand-split) for the one stem that +did not move whole). `--auto-apply` implies `--apply` and additionally rewrites +the findings that are mechanically safe: `from adcp.types.generated_poc.X import +Symbol` lines where every symbol has a known public alias on `adcp.types` are +imported from there instead of the domain path, and `Assets` numbered classes +that have a documented semantic alias (`Assets81 → VideoFormatAsset`, etc.) are +renamed. `flag_removed` findings always require human attention. The CLI exits 0 when all remaining findings are mechanical (or none); exits 1 when `flag_removed` findings remain for human review — wire it into CI to gate merges @@ -78,10 +83,10 @@ felt blast radius, work in this order: re-export to the rest of your codebase (e.g. a `_base.py` schema barrel). Fix those before anything else — most of your test-collection failures disappear. -2. **`adcp.types.generated_poc` reach-ins.** The codemod's per-symbol mapping - tells you the public alias for each (`ContextObject` → - `adcp.types.ContextObject`, etc.). Mechanical lookup once you know the - pattern. +2. **`adcp.types.generated_poc` reach-ins.** `--apply` renames the path to + `adcp.types.domains`, and the report's hint names the public alias where a + symbol has one (`ContextObject` → `adcp.types.ContextObject`, etc.) so + `--auto-apply` can import it from the flat surface instead. 3. **Numbered `Assets` imports.** Switch to the semantic alias from `adcp.types`. See [Numbered discriminated-union classes shifted](#numbered-discriminated-union-classes-shifted) below. @@ -317,6 +322,19 @@ result = await registry.lookup_brand("nike.com") manifest = result.brand # Only `.brand` ``` +## generated_poc brand split + +`brand.json` shares its basename with the `brand/*.json` task schemas, so the +pre-9.0 `adcp.types.generated_poc.brand` module *was* that discovery schema +(`Brand`, `LocalizedName`, 138 more), while `adcp.types.domains.brand` is the +aggregator over `domains/brand/.py`. The discovery classes are at +`adcp.types.domains.brand_discovery`. `adcp migrate v3-to-v4 --apply` resolves +each imported name to its half — a line that mixes the two becomes two import +statements — and flags what it cannot place: a bare module reference such as +`import adcp.types.generated_poc.brand as b` or a name neither half binds. +Rewrite those by hand with the two module paths above. The deprecated name +keeps serving both halves through 9.x; see `docs/types-9-migration.md`. + ## Numbered discriminated-union classes shifted `datamodel-code-generator` numbers variant classes in the order they appear in diff --git a/docs/types-9-migration.md b/docs/types-9-migration.md index cfc61bd00..43aa28bdd 100644 --- a/docs/types-9-migration.md +++ b/docs/types-9-migration.md @@ -226,7 +226,9 @@ deleted and the generated models go back to agreeing with their schema. +from adcp.types.domains.media_buy.package_request import PackageRequest ``` - `adcp migrate v3-to-v4` rewrites those lines. The old path also still + `adcp migrate v3-to-v4 --apply` rewrites those lines, and `--auto-apply` + additionally imports a name from `adcp.types` when every name on the line is + bound there. The old path also still resolves through the whole 9.x line, emitting a `DeprecationWarning` that names the new one — #1360 measured 110 such imports in a single production seller, and 9.0 does not break all of them at once. **It is removed in @@ -250,6 +252,9 @@ deleted and the generated models go back to agreeing with their schema. +from adcp.types.domains.brand_discovery import Brand, LocalizedName ``` + The codemod resolves this split per imported name — a line that mixes the + two halves becomes two import statements — and flags a bare + `adcp.types.generated_poc.brand` module reference it cannot place. The deprecated name keeps serving both halves for the 9.x line, so an unmigrated `from adcp.types.generated_poc.brand import Brand` still works and still returns the canonical class. It is the one deprecated name that is a diff --git a/src/adcp/migrate/__main__.py b/src/adcp/migrate/__main__.py index 8b997e4ce..b984371a5 100644 --- a/src/adcp/migrate/__main__.py +++ b/src/adcp/migrate/__main__.py @@ -23,8 +23,9 @@ def main() -> int: "usage: python -m adcp.migrate [options]\n" "\n" "Migrations:\n" - " v3-to-v4 Rewrite Asset → Content and flag\n" - " removed-type usages (BrandManifest, DeliverTo, etc).\n" + " v3-to-v4 Rewrite Asset → Content, rename\n" + " adcp.types.generated_poc imports to adcp.types.domains,\n" + " and flag removed-type usages (BrandManifest, DeliverTo, etc).\n" "\n" "Run `python -m adcp.migrate --help` for per-migration options.\n", file=sys.stderr, diff --git a/src/adcp/migrate/v3_to_v4.py b/src/adcp/migrate/v3_to_v4.py index 1817ac3f3..6c80f75e0 100644 --- a/src/adcp/migrate/v3_to_v4.py +++ b/src/adcp/migrate/v3_to_v4.py @@ -11,11 +11,19 @@ * **Applied**: direct name rewrites (``AudioAsset`` → ``AudioContent`` etc). The 9 rename targets are distinctive enough that word-boundary - regex is safe; sellers should still review the diff. -* **Flagged**: removed types, numbered ``Assets`` imports, - ``adcp.types.generated_poc`` imports. These don't rewrite — the - seller has to choose the replacement (e.g. ``BrandManifest`` → - ``BrandReference(domain=...)`` depends on call-site context). + regex is safe; sellers should still review the diff. The + ``adcp.types.generated_poc`` → ``adcp.types.domains`` import-path + rename is applied the same way (#1417): the generated tree moved + there with its module stems unchanged, and the old path returns the + same objects, so the rewrite is mechanical. The one stem that split, + ``brand``, is resolved per imported name against the two halves it + became (``domains.brand_discovery`` for ``brand.json``'s classes, + ``domains.brand`` for the task-schema aggregator) and flagged when + it cannot be. +* **Flagged**: removed types and numbered ``Assets`` imports. + These don't rewrite — the seller has to choose the replacement + (e.g. ``BrandManifest`` → ``BrandReference(domain=...)`` depends on + call-site context). Invocation:: @@ -25,14 +33,16 @@ python -m adcp.migrate v3-to-v4 ./src --json # structured report The dry run is the default — you always see what would change before -anything moves. ``--apply`` rewrites the 9 ``Asset`` renames in -place. ``--auto-apply`` implies ``--apply`` and additionally rewrites -``flag_private`` findings whose target symbol is a known public alias in -``adcp.types``, and ``flag_numbered`` findings with a documented semantic -alias (``Assets81`` → ``VideoFormatAsset``, etc.). ``flag_removed`` -findings always require human review and remain flagged even with -``--auto-apply``. Commit your tree before running either write mode so -``git diff`` is your review view. +anything moves. ``--apply`` rewrites the 9 ``Asset`` renames and +the ``generated_poc`` → ``domains`` import paths in place. +``--auto-apply`` implies ``--apply`` and additionally lifts a +``generated_poc`` import to ``adcp.types`` when every name on the line is +a known public alias there (the flat surface is the first choice; a +domain path is for a name it cannot bind), and rewrites ``flag_numbered`` +findings with a documented semantic alias (``Assets81`` → +``VideoFormatAsset``, etc.). ``flag_removed`` findings always require +human review and remain flagged even with ``--auto-apply``. Commit your +tree before running either write mode so ``git diff`` is your review view. .. important:: The codemod matches identifiers textually (word-boundary regex, not @@ -50,6 +60,7 @@ import argparse import importlib +import importlib.util import json import re import sys @@ -126,14 +137,237 @@ } +# The pre-9.0 generated tree's import path and where it moved (#1417). Every +# module stem under the first is served by the same stem under the second, so +# the rewrite is a prefix rename, with one exception: a root discovery schema +# whose basename collided with a task-schema directory split into +# ``_discovery`` (its own classes) and ```` (the aggregator). +# ``adcp.types._generated_poc_alias`` is the runtime half of the same contract. +DEPRECATED_TYPES_ROOT = "adcp.types.generated_poc" +CANONICAL_TYPES_ROOT = "adcp.types.domains" +ROOT_DISCOVERY_SUFFIX = "_discovery" + +# Any ``adcp.types.generated_poc[.a.b]`` reference — an ``import`` statement, +# a dotted attribute access, the module half of a multi-line ``from`` import. +_GENERATED_POC_REF = re.compile(r"\badcp\.types\.generated_poc(?P(?:\.\w+)*)") + # Private-module imports that shouldn't appear in downstream code. PRIVATE_IMPORT_PATHS: dict[str, str] = { - "adcp.types.generated_poc": ( - "private module — import from adcp.types (stable public API) instead" + DEPRECATED_TYPES_ROOT: ( + "deprecated path — the generated tree lives at adcp.types.domains under " + "the same module stems; --apply renames the prefix" ), } +def _split_halves(stem: str) -> tuple[str, str] | None: + """``("adcp.types.domains.brand_discovery", "adcp.types.domains.brand")`` for a + split domain stem, ``None`` for a stem that moved whole. + + Answered by what the installed SDK ships rather than a table here, so a + schema added to ``scripts/generate_types.py``'s ``ROOT_DISCOVERY_SCHEMAS`` + tomorrow is resolved without touching this file. The ``_discovery`` suffix is + the generator's own, the same one ``adcp.types._generated_poc_alias`` serves + at runtime; ``tests/test_migrate_v3_to_v4.py`` pins the two spellings equal. + """ + if not stem or "." in stem: + return None + canonical = f"{CANONICAL_TYPES_ROOT}.{stem}" + discovery = f"{canonical}{ROOT_DISCOVERY_SUFFIX}" + try: + if importlib.util.find_spec(discovery) is None: + return None + except (ImportError, AttributeError, ValueError): + return None + return discovery, canonical + + +def _resolve_split_symbol(stem: str, symbol: str) -> str | None: + """The half of a split stem that binds ``symbol``; discovery half first. + + Discovery first because that is the surface the pre-9.0 module had — a name + both halves bind (``Asset``, ``Fonts``) meant the discovery schema's class + to code written against ``generated_poc.brand``. + """ + halves = _split_halves(stem) + if halves is None: + return None + for module_name in halves: + try: + module = importlib.import_module(module_name) + except ImportError: + continue + if hasattr(module, symbol): + return module_name + return None + + +def _split_hint(stem: str, symbols: list[str]) -> str: + halves = _split_halves(stem) or ( + f"{CANONICAL_TYPES_ROOT}.{stem}_discovery", + f"{CANONICAL_TYPES_ROOT}.{stem}", + ) + names = ", ".join(symbols) if symbols else "the names used" + return ( + f"`{stem}` split in two in 9.0: {halves[0]} holds the classes the old module " + f"declared and {halves[1]} the ones its domain aggregates. Resolve {names} by " + f"hand (see docs/types-9-migration.md)." + ) + + +def _import_symbols(symbols_text: str) -> tuple[list[str], list[str]]: + """``("A", "B as C")`` → raw entries and the names they import.""" + raws = [raw.strip() for raw in symbols_text.split(",") if raw.strip()] + return raws, [raw.split(" as ", 1)[0].strip() for raw in raws] + + +def _plan_from_import( + line: str, match: re.Match[str] +) -> tuple[dict[str, list[str]] | None, str | None]: + """Where a ``from adcp.types.generated_poc. import ...`` line goes. + + Returns ``(targets, None)`` — new module path → raw import entries, one key + for a whole-stem rename, two when a split stem's names resolve to different + halves — or ``(None, hint)`` when a split stem's names cannot be resolved, + or the line would have to be split and is not alone on its line. + """ + module = match.group("module") or "" + raws, symbols = _import_symbols(match.group("symbols")) + if not module: + # ``from adcp.types.generated_poc import brand`` names subpackages; a + # split stem among them has no single new module. + split = [symbol for symbol in symbols if _split_halves(symbol) is not None] + if split: + return None, _split_hint(split[0], []) + return {CANONICAL_TYPES_ROOT: raws}, None + if _split_halves(module) is None: + return {f"{CANONICAL_TYPES_ROOT}.{module}": raws}, None + grouped: dict[str, list[str]] = {} + unresolved: list[str] = [] + for raw, symbol in zip(raws, symbols, strict=True): + target = _resolve_split_symbol(module, symbol) + if target is None: + unresolved.append(symbol) + else: + grouped.setdefault(target, []).append(raw) + if unresolved or not grouped: + return None, _split_hint(module, unresolved) + if len(grouped) > 1 and line[: match.start()].strip(): + # Two import statements are needed and the ``from`` shares its line + # with other code; a textual split would corrupt it. + return None, _split_hint(module, symbols) + return grouped, None + + +def _is_split_reference(rest: str) -> bool: + """``.brand`` (exactly the split stem) is ambiguous; ``.brand.x`` is not.""" + return rest.count(".") == 1 and _split_halves(rest[1:]) is not None + + +def _lift_hint(module: str, symbols: list[str]) -> str | None: + """Name the ``adcp.types`` home of any symbol that has one, for the report.""" + lifted = [ + f"{symbol} → {replacement}" + for symbol in symbols + if (replacement := _generated_symbol_replacement(module, symbol)) is not None + ] + if not lifted: + return None + return "also bound on the flat surface (--auto-apply imports from there): " + ", ".join(lifted) + + +def _prefix_rename_findings(path: Path, lineno: int, line: str) -> list[Finding]: + """The ``rename_import`` (or split ``flag_private``) findings for one line.""" + findings: list[Finding] = [] + column = line.index(DEPRECATED_TYPES_ROOT) + 1 + from_match = _GENERATED_POC_FROM_IMPORT.search(line) + if from_match is not None: + module = from_match.group("module") or "" + _raws, symbols = _import_symbols(from_match.group("symbols")) + targets, hint = _plan_from_import(line, from_match) + before = f"{DEPRECATED_TYPES_ROOT}.{module}" if module else DEPRECATED_TYPES_ROOT + if targets is None: + findings.append( + Finding( + kind="flag_private", + path=str(path), + line=lineno, + column=column, + before=before, + hint=hint, + migration_anchor="generated_poc-brand-split", + ) + ) + return findings + for target in targets: + findings.append( + Finding( + kind="rename_import", + path=str(path), + line=lineno, + column=column, + before=before, + after=target, + hint=_lift_hint(module, symbols), + ) + ) + return findings + for match in _GENERATED_POC_REF.finditer(line): + rest = match.group("rest") + if _is_split_reference(rest): + findings.append( + Finding( + kind="flag_private", + path=str(path), + line=lineno, + column=match.start() + 1, + before=match.group(0), + hint=_split_hint(rest[1:], []), + migration_anchor="generated_poc-brand-split", + ) + ) + else: + findings.append( + Finding( + kind="rename_import", + path=str(path), + line=lineno, + column=match.start() + 1, + before=match.group(0), + after=CANONICAL_TYPES_ROOT + rest, + ) + ) + return findings + + +def _prefix_rename_line(text_line: str) -> str: + """Apply the import-path rename to one line; a flagged reference is left alone.""" + from_match = _GENERATED_POC_FROM_IMPORT.search(text_line) + if from_match is not None: + targets, _hint = _plan_from_import(text_line, from_match) + if targets is None: + return text_line + head = text_line[: from_match.start()] + tail = text_line[from_match.end() :] + if len(targets) == 1: + (target,) = targets + symbols_text = from_match.group("symbols") + return f"{head}from {target} import {symbols_text}{tail}" + newline = "\r\n" if tail.endswith("\r\n") else "\n" + statements = [ + f"{head}from {target} import {', '.join(raws)}" for target, raws in targets.items() + ] + return newline.join(statements) + tail + return _GENERATED_POC_REF.sub( + lambda match: ( + match.group(0) + if _is_split_reference(match.group("rest")) + else CANONICAL_TYPES_ROOT + match.group("rest") + ), + text_line, + ) + + # Per-symbol mapping for the most common ``generated_poc`` reach-ins # salesagent surfaced during their v3→v4 experiment (and any other # adopter would hit). The codemod scans for ``from @@ -322,9 +556,9 @@ def _unsafe_replacement_hint(module: str, symbol: str, replacement: str) -> str: class Finding: """One migration finding — either an applied rename or a manual TODO.""" - # Valid kind values: "rename" | "auto_applied" | "flag_removed" | - # "flag_private" | "flag_numbered" | "flag_attribute" | - # "flag_enum_value" + # Valid kind values: "rename" | "rename_import" | "auto_applied" | + # "flag_removed" | "flag_private" | "flag_numbered" | + # "flag_attribute" | "flag_enum_value" kind: str path: str line: int @@ -346,7 +580,7 @@ class Report: rewritten_files: int = 0 def add(self, finding: Finding) -> None: - if finding.kind == "rename": + if finding.kind in ("rename", "rename_import"): self.applied.append(finding) elif finding.kind == "auto_applied": self.auto_applied.append(finding) @@ -458,6 +692,7 @@ def scan_file( updated = original rename_hits = False auto_apply_hits = False # any numbered or private-import rewrites queued + rename_import_hits = False # any generated_poc → domains path renames queued for lineno, line in enumerate(original.splitlines(), start=1): # Pre-pass: when this line is a single-line ``generated_poc`` @@ -548,125 +783,54 @@ def scan_file( ) ) - # adcp.types.generated_poc imports. + # adcp.types.generated_poc references (#1417). # - # When the line is a single-line + # Under ``--auto-apply``, a single-line # ``from adcp.types.generated_poc. import `` - # emit one per-symbol Finding. For symbols in - # GENERATED_POC_SYMBOL_MAP the Finding carries the public alias; - # unknown symbols get the generic "private module" flag so they - # still surface (fixing the prior silent-drop on mixed lines). + # whose every symbol is a known public alias (or a mapped numbered + # asset) is lifted to ``adcp.types`` — one ``auto_applied`` Finding + # per symbol. Every other reference is a prefix rename to + # ``adcp.types.domains`` (``rename_import``, applied by ``--apply``), + # except a bare split stem (``generated_poc.brand``) whose names + # cannot be placed, which is flagged with the two halves named. # - # Under ``--auto-apply`` the per-symbol findings are promoted to - # ``auto_applied`` only when ALL symbols on the line are in the - # map — a mixed line cannot be safely rewritten without splitting - # the import statement, so it remains flagged. - # - # Numbered-Assets imports (e.g. ``import Assets81``) appear here - # too. Those are handled by the numbered pass above and will be - # fixed by the post-scan import-path rewrite; suppress the extra - # generic flag so the report doesn't double-count them. - for private_path, hint in PRIVATE_IMPORT_PATHS.items(): - if private_path not in line: - continue - col = line.index(private_path) + 1 + # Numbered-Assets imports appear on these lines too; the numbered + # pass above owns their Findings. + if DEPRECATED_TYPES_ROOT in line: from_match = _GENERATED_POC_FROM_IMPORT.search(line) - if from_match: + lifted = False + if from_match is not None and auto_apply and not line_is_mixed_unsafe_import: module = from_match.group("module") or "" - raw_symbols = [s.strip() for s in from_match.group("symbols").split(",")] - parsed: list[tuple[str, str | None]] = [] - for raw in raw_symbols: - raw = raw.strip() - if not raw: - continue - symbol = raw.split(" as ")[0].strip() - if not symbol: - continue - parsed.append((symbol, _generated_symbol_replacement(module, symbol))) - - if not parsed: + _raws, symbols = _import_symbols(from_match.group("symbols")) + lifted = bool(symbols) + for symbol in symbols: + replacement = _generated_symbol_replacement(module, symbol) + if replacement is None: + continue # a mapped numbered asset; the numbered pass reported it + sym_col = line.find(symbol, from_match.start(1)) + 1 findings.append( Finding( - kind="flag_private", + kind="auto_applied", path=str(path), line=lineno, - column=col, - before=private_path, - hint=hint, + column=( + sym_col if sym_col > 0 else line.index(DEPRECATED_TYPES_ROOT) + 1 + ), + before=symbol, + after=replacement, + hint=( + "deprecated path — import " + f"{replacement.rsplit('.', 1)[-1]} from " + "adcp.types (stable public API) instead" + ), ) ) - continue - - all_known = all( - repl is not None or (auto_apply and symbol in NUMBERED_ASSETS_RENAMES) - for symbol, repl in parsed - ) - - for symbol, replacement in parsed: - sym_col = line.find(symbol, from_match.start(1)) + 1 - if sym_col <= 0: - sym_col = col - if replacement is not None: - kind = "auto_applied" if (auto_apply and all_known) else "flag_private" - if kind == "auto_applied": - auto_apply_hits = True - findings.append( - Finding( - kind=kind, - path=str(path), - line=lineno, - column=sym_col, - before=symbol, - after=replacement, - hint=( - "private module — import " - f"{replacement.rsplit('.', 1)[-1]} from " - "adcp.types (stable public API) instead" - ), - ) - ) - else: - # Unknown symbol. Suppress the generic flag when - # auto_apply is active and the symbol is a numbered - # asset that will be renamed by the other pass — - # the import-path fix covers it. - if auto_apply and symbol in NUMBERED_ASSETS_RENAMES: - continue - proposed = _proposed_generated_symbol_replacement(module, symbol) - if proposed is not None: - findings.append( - Finding( - kind="flag_private", - path=str(path), - line=lineno, - column=sym_col, - before=symbol, - hint=_unsafe_replacement_hint(module, symbol, proposed), - ) - ) - continue - findings.append( - Finding( - kind="flag_private", - path=str(path), - line=lineno, - column=sym_col, - before=private_path, - hint=hint, - ) - ) - else: - # Multiline import, star import, or regex mismatch. - findings.append( - Finding( - kind="flag_private", - path=str(path), - line=lineno, - column=col, - before=private_path, - hint=hint, - ) - ) + auto_apply_hits = True + if not lifted: + for finding in _prefix_rename_findings(path, lineno, line): + findings.append(finding) + if finding.kind == "rename_import": + rename_import_hits = True # Removed attribute accesses (.brand_manifest etc.). Regex with # trailing word boundary prevents false-positives on @@ -773,6 +937,16 @@ def scan_file( updated = "".join(new_lines) needs_write = True + if apply_changes and rename_import_hits and DEPRECATED_TYPES_ROOT in updated: + # After any lift to ``adcp.types`` above, every remaining + # ``generated_poc`` reference is a prefix rename (or a flagged split + # stem, which ``_prefix_rename_line`` leaves alone). + updated = "".join( + _prefix_rename_line(text_line) if DEPRECATED_TYPES_ROOT in text_line else text_line + for text_line in updated.splitlines(keepends=True) + ) + needs_write = True + if needs_write: return findings, updated return findings, None @@ -804,11 +978,13 @@ def _format_text_report(report: Report, *, apply_changes: bool, auto_apply: bool lines.append(f"adcp migrate v3-to-v4 — scanned {report.scanned_files} files") lines.append("") - if report.applied: - lines.append(f"Asset renames {mode}: {len(report.applied)}") + asset_renames = [f for f in report.applied if f.kind == "rename"] + import_renames = [f for f in report.applied if f.kind == "rename_import"] + if asset_renames: + lines.append(f"Asset renames {mode}: {len(asset_renames)}") # Group by (before, after) for a compact summary. by_rename: dict[str, dict[str, list[Finding]]] = {} - for f in report.applied: + for f in asset_renames: by_rename.setdefault(f.before, {}).setdefault(f.after or "?", []).append(f) for before, after_map in sorted(by_rename.items()): for after, hits in sorted(after_map.items()): @@ -822,6 +998,25 @@ def _format_text_report(report: Report, *, apply_changes: bool, auto_apply: bool else: lines.append("No asset renames needed.") + if import_renames: + lines.append("") + lines.append(f"Import path renames {mode}: {len(import_renames)}") + by_path: dict[str, dict[str, list[Finding]]] = {} + for f in import_renames: + by_path.setdefault(f.before, {}).setdefault(f.after or "?", []).append(f) + for before, after_map in sorted(by_path.items()): + for after, hits in sorted(after_map.items()): + lines.append( + f" {before} → {after} ({len(hits)} hit{'s' if len(hits) != 1 else ''})" + ) + hint = hits[0].hint + if hint: + lines.append(f" → {hint}") + for f in hits[:5]: + lines.append(f" {f.path}:{f.line}:{f.column}") + if len(hits) > 5: + lines.append(f" … and {len(hits) - 5} more") + if report.auto_applied: lines.append("") lines.append(f"Safe rewrites {mode}: {len(report.auto_applied)}") @@ -875,11 +1070,11 @@ def _format_text_report(report: Report, *, apply_changes: bool, auto_apply: bool lines.append(f"Rewrote {report.rewritten_files} files in place.") lines.append("Review with `git diff` before committing.") - if not auto_apply and any(f.kind in ("flag_private", "flag_numbered") for f in report.flagged): + if not auto_apply and any(f.kind == "flag_numbered" for f in report.flagged): lines.append("") lines.append( - "Tip: rerun with --auto-apply to mechanically fix the " - "flag_private and flag_numbered findings above." + "Tip: rerun with --auto-apply to mechanically fix the flag_numbered " + "findings above and import from adcp.types where a name is bound there." ) return "\n".join(lines) @@ -903,8 +1098,9 @@ def _format_text_report(report: Report, *, apply_changes: bool, auto_apply: bool "scanned_files": int, "rewritten_files": int, "applied": [ - {"kind": "rename", "path": str, "line": int, "column": int, - "before": str, "after": str, "hint": null, "migration_anchor": null} + {"kind": "rename" | "rename_import", "path": str, "line": int, + "column": int, "before": str, "after": str, "hint": str | null, + "migration_anchor": null} ], "auto_applied": [ {"kind": "auto_applied", "path": str, "line": int, "column": int, @@ -918,7 +1114,10 @@ def _format_text_report(report: Report, *, apply_changes: bool, auto_apply: bool ] } -``auto_applied`` is an additive field (v1, no version bump needed). +``auto_applied`` is an additive field (v1, no version bump needed), and so +is the ``rename_import`` kind in ``applied`` (#1417): the +``adcp.types.generated_poc`` → ``adcp.types.domains`` import-path rename, +reported with the old module path in ``before`` and the new one in ``after``. Parsers that don't know about it receive an empty array in non-``--auto-apply`` runs and can safely ignore it. Entries in ``flagged`` always require human attention regardless of what ``auto_applied`` contains. @@ -987,7 +1186,8 @@ def main(argv: list[str] | None = None) -> int: parser = argparse.ArgumentParser( prog="adcp.migrate v3-to-v4", description=( - "Rewrite adcp 3.x → 4.0 ``Asset`` → ``Content`` renames " + "Rewrite adcp 3.x → 4.0 ``Asset`` → ``Content`` renames, " + "rename adcp.types.generated_poc imports to adcp.types.domains, " "and flag usages of removed types. " "Exits 0 when all findings are mechanical (or none); " "exits 1 when flag_removed findings remain for human review; " @@ -1003,10 +1203,10 @@ def main(argv: list[str] | None = None) -> int: "--apply", action="store_true", help=( - "Rewrite files in place. Default is dry-run (report only). " - "Commit your tree first so `git diff` is your review view. " - "See also --auto-apply to also mechanically fix flag_private " - "and flag_numbered findings." + "Rewrite files in place: the Asset renames and the " + "adcp.types.generated_poc → adcp.types.domains import paths. " + "Default is dry-run (report only). Commit your tree first so " + "`git diff` is your review view. See also --auto-apply." ), ) parser.add_argument( @@ -1015,8 +1215,9 @@ def main(argv: list[str] | None = None) -> int: dest="auto_apply", help=( "Rewrite files in place (implies --apply) and additionally " - "auto-apply safe import rewrites: flag_private findings " - "whose target symbol exists on adcp.types, and flag_numbered " + "auto-apply safe import rewrites: a generated_poc import " + "whose every symbol is bound on adcp.types is imported from " + "there instead of the domain path, and flag_numbered " "findings with a documented semantic alias (Assets81 → " "VideoFormatAsset, etc.). flag_removed findings always " "require human review and remain flagged; exit code 1 when " diff --git a/src/adcp/types/__init__.py b/src/adcp/types/__init__.py index d7cc3cde9..7cdab8a83 100644 --- a/src/adcp/types/__init__.py +++ b/src/adcp/types/__init__.py @@ -21,10 +21,18 @@ from adcp.types.buyer import GetProductsRequest from adcp.types.seller import Offering, PropertyList -IMPORTANT: Never import directly from adcp.types.domains or -adcp.types._generated. These are internal modules regenerated from -upstream schemas. Only import from adcp.types (this module), one of the -partial modules above, or adcp. +When the flat namespace cannot bind a name — two schemas declare the same +class name, or a numbered variant has no semantic alias — import it by its +schema path under ``adcp.types.domains``, which mirrors the bundle's layout +and is where codegen defines every class (see docs/type-surface.md): + + from adcp.types.domains.creative.list_creatives_response import Creative + +Never import from adcp.types._generated, adcp.types._eager or any other +underscore-prefixed module: those are internal and change without notice. +The pre-9.0 ``adcp.types.generated_poc`` path still resolves with a +``DeprecationWarning`` and is removed in v10; ``adcp migrate v3-to-v4 +--apply`` rewrites it to ``adcp.types.domains``. Type Coercion: Request types accept flexible input for developer ergonomics: diff --git a/src/adcp/types/_generated_poc_alias.py b/src/adcp/types/_generated_poc_alias.py index 7be3a3981..12203e64b 100644 --- a/src/adcp/types/_generated_poc_alias.py +++ b/src/adcp/types/_generated_poc_alias.py @@ -11,9 +11,10 @@ -from adcp.types.generated_poc.media_buy.package_request import PackageRequest +from adcp.types.domains.media_buy.package_request import PackageRequest -``adcp migrate v3-to-v4`` rewrites those lines; ``adcp.types`` remains the -first choice, and a domain path is for a name the flat namespace cannot bind, -as ``docs/type-surface.md`` describes. +``adcp migrate v3-to-v4 --apply`` rewrites those lines (and resolves the +split stem below per imported name); ``adcp.types`` remains the first choice, +and a domain path is for a name the flat namespace cannot bind, as +``docs/type-surface.md`` describes. **A root discovery schema is the exception, and a prefix rename loses it.** ``scripts/generate_types.py``'s ``ROOT_DISCOVERY_SCHEMAS`` names every schema @@ -258,14 +259,15 @@ def find_spec( advice = ( f"import {canonical} instead. The module stems are " f"unchanged, so this is a prefix rename, and " - f"`adcp migrate v3-to-v4` rewrites it." + f"`adcp migrate v3-to-v4 --apply` rewrites it." ) else: advice = ( f"import {discovery} for the classes this module declared " f"and {canonical} for the ones its domain aggregates — this " f"stem split in two, so it is the one case that is not a " - f"prefix rename. See docs/types-9-migration.md." + f"prefix rename; `adcp migrate v3-to-v4 --apply` resolves each " + f"imported name to its half. See docs/types-9-migration.md." ) warnings.warn( f"{fullname} is deprecated since adcp 9.0 and is removed in " diff --git a/tests/test_migrate_v3_to_v4.py b/tests/test_migrate_v3_to_v4.py index 4e73c684d..78124be78 100644 --- a/tests/test_migrate_v3_to_v4.py +++ b/tests/test_migrate_v3_to_v4.py @@ -161,12 +161,10 @@ def test_bare_assets_is_not_flagged_as_numbered(tmp_path: Path) -> None: assert numbered == [] -def test_flags_generated_poc_imports_unknown_symbol_falls_back_to_generic_hint( - tmp_path: Path, -) -> None: - """A ``generated_poc`` import for a symbol not in the per-symbol map - falls back to the generic 'private module' flag — still surfaces the - issue, adopter does the lookup.""" +def test_generated_poc_import_of_an_unknown_symbol_is_a_prefix_rename(tmp_path: Path) -> None: + """A ``generated_poc`` import whose symbol has no ``adcp.types`` home is + still mechanical: the module moved to ``adcp.types.domains`` under the same + stem, so the line is a ``rename_import``, not something for a human (#1417).""" _write( tmp_path, "code.py", @@ -175,17 +173,20 @@ def test_flags_generated_poc_imports_unknown_symbol_falls_back_to_generic_hint( report = v3_to_v4.run(tmp_path, apply_changes=False) - private = [f for f in report.flagged if f.kind == "flag_private"] - assert len(private) == 1 - assert private[0].before == "adcp.types.generated_poc" + assert report.flagged == [] + renames = [f for f in report.applied if f.kind == "rename_import"] + assert len(renames) == 1 + assert renames[0].before == "adcp.types.generated_poc.core.something" + assert renames[0].after == "adcp.types.domains.core.something" + assert renames[0].hint is None -def test_flags_generated_poc_imports_per_symbol_mapping(tmp_path: Path) -> None: +def test_generated_poc_import_hint_names_the_flat_surface_home(tmp_path: Path) -> None: """Round-5 adopter feedback (salesagent v3→v4 experiment): the ``generated_poc`` flag-only output forced 82 of 156 findings into - hand-grep territory. Each known reach-in now emits an explicit - "Symbol → adcp.types.Symbol" replacement so adopters apply the fix - without leaving the codemod report.""" + hand-grep territory. The line is now a mechanical ``rename_import`` to the + domain path, and its hint still names the ``adcp.types`` home of each known + symbol so adopters can pick ``--auto-apply`` without leaving the report.""" _write( tmp_path, "code.py", @@ -197,26 +198,20 @@ def test_flags_generated_poc_imports_per_symbol_mapping(tmp_path: Path) -> None: report = v3_to_v4.run(tmp_path, apply_changes=False) - private = [f for f in report.flagged if f.kind == "flag_private"] - by_symbol = {f.before: f for f in private} - - # Each known symbol gets a per-symbol replacement, NOT the generic - # "adcp.types.generated_poc" flag. - assert by_symbol["ContextObject"].after == "adcp.types.ContextObject" - assert by_symbol["BrandReference"].after == "adcp.types.BrandReference" - assert by_symbol["MediaBuyStatus"].after == "adcp.types.MediaBuyStatus" + assert report.flagged == [] + by_line = {f.line: f for f in report.applied if f.kind == "rename_import"} + assert by_line[1].after == "adcp.types.domains.core.context" + assert "ContextObject → adcp.types.ContextObject" in (by_line[1].hint or "") + assert "BrandReference → adcp.types.BrandReference" in (by_line[2].hint or "") + assert "MediaBuyStatus → adcp.types.MediaBuyStatus" in (by_line[3].hint or "") # ``import Error as AdCPResponseError`` — codemod keys off the LHS # canonical name, ignoring the local alias. - assert by_symbol["Error"].after == "adcp.types.Error" - # The generic private-module flag MUST NOT also fire when the - # per-symbol mapping handled the line — would double-count and - # confuse the report. - assert "adcp.types.generated_poc" not in by_symbol + assert "Error → adcp.types.Error" in (by_line[4].hint or "") -def test_flags_generated_poc_multiple_symbols_one_line(tmp_path: Path) -> None: - """``from adcp.types.generated_poc.core.x import A, B, C`` emits - one Finding per symbol so the report surfaces every replacement.""" +def test_generated_poc_import_hint_covers_every_symbol_on_the_line(tmp_path: Path) -> None: + """``from adcp.types.generated_poc.core.x import A, B`` names both + replacements in the one ``rename_import`` hint.""" _write( tmp_path, "code.py", @@ -226,12 +221,11 @@ def test_flags_generated_poc_multiple_symbols_one_line(tmp_path: Path) -> None: report = v3_to_v4.run(tmp_path, apply_changes=False) - private = [f for f in report.flagged if f.kind == "flag_private"] - by_symbol = {f.before: f.after for f in private} - assert by_symbol == { - "VendorPricingOption1": "adcp.types.CpmVendorPricingOption", - "VendorPricingOption2": "adcp.types.PercentOfMediaVendorPricingOption", - } + (rename,) = [f for f in report.applied if f.kind == "rename_import"] + assert rename.after == "adcp.types.domains.core.vendor_pricing_option" + assert rename.hint is not None + assert "VendorPricingOption1 → adcp.types.CpmVendorPricingOption" in rename.hint + assert "VendorPricingOption2 → adcp.types.PercentOfMediaVendorPricingOption" in rename.hint def test_generated_poc_symbol_map_covers_publicly_exported_names() -> None: @@ -758,17 +752,25 @@ def test_auto_apply_preserves_crlf_after_source_scoped_import(tmp_path: Path) -> def test_auto_apply_does_not_guess_colliding_source_variant(tmp_path: Path) -> None: + """``core.product_filters.ProductFilters`` is not ``adcp.types.ProductFilters`` + (the public name is the canonical model), so --auto-apply must not lift it. + The line still leaves the deprecated path: it is prefix-renamed to the + domain module, where the class actually lives.""" path = _write( tmp_path, "code.py", "from adcp.types.generated_poc.core.product_filters import ProductFilters\n", ) report = v3_to_v4.run(tmp_path, apply_changes=True, auto_apply=True) - assert "adcp.types.generated_poc.core.product_filters" in path.read_text() - finding = next(f for f in report.flagged if f.before == "ProductFilters") - assert finding.hint is not None - assert finding.hint.startswith("SKIP: source") - assert "manual rewrite required" in finding.hint + + rewritten = path.read_text() + assert rewritten == "from adcp.types.domains.core.product_filters import ProductFilters\n" + assert report.auto_applied == [] + assert report.flagged == [] + (rename,) = [f for f in report.applied if f.kind == "rename_import"] + assert rename.after == "adcp.types.domains.core.product_filters" + # Nothing claims a flat-surface home it does not have. + assert rename.hint is None def test_auto_apply_rewrites_multi_symbol_all_known_line(tmp_path: Path) -> None: @@ -804,9 +806,11 @@ def test_auto_apply_preserves_as_alias(tmp_path: Path) -> None: assert "from adcp.types import Error as AdCPError" in rewritten -def test_auto_apply_mixed_line_not_rewritten(tmp_path: Path) -> None: - """A line with at least one unknown symbol MUST NOT be auto-applied. - The known symbol is still flagged (not silently dropped).""" +def test_auto_apply_mixed_line_is_prefix_renamed_not_lifted(tmp_path: Path) -> None: + """A line with at least one symbol that is not bound on ``adcp.types`` is + not lifted there — lifting one name while leaving another behind would + import a public name from a module that does not export it. The prefix + rename has no such hazard, so the line still leaves the deprecated path.""" path = _write( tmp_path, "code.py", @@ -815,22 +819,12 @@ def test_auto_apply_mixed_line_not_rewritten(tmp_path: Path) -> None: report = v3_to_v4.run(tmp_path, apply_changes=True, auto_apply=True) rewritten = path.read_text() - assert "adcp.types.generated_poc" in rewritten, "mixed line must NOT be rewritten" - - # Known symbol must still appear in flagged (not silently dropped). - flagged_symbols = {f.before for f in report.flagged if f.kind == "flag_private"} - assert "BrandReference" in flagged_symbols - - # Unknown symbol gets a generic flag too (silent-drop bug is fixed). - generic_flags = [ - f - for f in report.flagged - if f.kind == "flag_private" and f.before == "adcp.types.generated_poc" - ] - assert len(generic_flags) >= 1 - - # Nothing should be in auto_applied. + assert rewritten == "from adcp.types.domains.core.brand_ref import BrandReference, Unknown\n" assert report.auto_applied == [] + assert report.flagged == [] + (rename,) = [f for f in report.applied if f.kind == "rename_import"] + # The known symbol's flat-surface home is still surfaced, not silently dropped. + assert "BrandReference → adcp.types.BrandReference" in (rename.hint or "") # --------------------------------------------------------------------------- @@ -874,17 +868,17 @@ def test_auto_apply_rewrites_numbered_asset_and_fixes_import_path(tmp_path: Path def test_auto_apply_unknown_numbered_stays_flagged(tmp_path: Path) -> None: - """A numbered asset not in NUMBERED_ASSETS_RENAMES (e.g. Assets149) - is not auto-applied and remains in report.flagged.""" + """A numbered asset not in NUMBERED_ASSETS_RENAMES (e.g. Assets149) is not + renamed and remains in report.flagged; its import path still moves off the + deprecated prefix, which is a separate, mechanical rename.""" path = _write( tmp_path, "code.py", "from adcp.types.generated_poc.bundled.x import Assets149\n", ) - original = path.read_text() report = v3_to_v4.run(tmp_path, apply_changes=True, auto_apply=True) - assert path.read_text() == original, "unmapped numbered asset must not be rewritten" + assert path.read_text() == "from adcp.types.domains.bundled.x import Assets149\n" numbered = [f for f in report.flagged if f.kind == "flag_numbered"] assert any(f.before == "Assets149" for f in numbered) assert report.auto_applied == [] @@ -920,8 +914,8 @@ def test_auto_apply_mixed_numbered_known_unknown_does_not_corrupt_import( """Regression: a generated_poc import mixing a known numbered asset (Assets81) with an unknown numbered asset (Assets149) MUST NOT silently rewrite Assets81 alone — that would leave VideoFormatAsset - imported from a private module path that doesn't export it - (guaranteed ImportError in adopter source).""" + imported from a module path that doesn't export it (guaranteed + ImportError in adopter source). The module path itself still moves.""" path = _write( tmp_path, "code.py", @@ -929,9 +923,8 @@ def test_auto_apply_mixed_numbered_known_unknown_does_not_corrupt_import( ) report = v3_to_v4.run(tmp_path, apply_changes=True, auto_apply=True) - # File content must be untouched on the unsafe-mixed line. rewritten = path.read_text() - assert "from adcp.types.generated_poc.core.format import Assets81, Assets149" in rewritten + assert rewritten == "from adcp.types.domains.core.format import Assets81, Assets149\n" assert "VideoFormatAsset" not in rewritten # Findings: Assets81 and Assets149 are both flagged for review (the @@ -1106,11 +1099,11 @@ def test_text_report_shows_tip_when_safe_findings_remain( tmp_path: Path, capsys: pytest.CaptureFixture[str] ) -> None: """Without --auto-apply the text report hints at --auto-apply when - flag_private or flag_numbered findings are present.""" + flag_numbered findings are present.""" _write( tmp_path, "code.py", - "from adcp.types.generated_poc.core.context import ContextObject\n", + "from adcp.types.generated_poc.core.format import Assets81\n", ) v3_to_v4.main([str(tmp_path)]) out = capsys.readouterr().out @@ -1138,9 +1131,10 @@ def test_text_report_no_tip_when_auto_apply_active( def test_mixed_line_unknown_symbol_not_silently_dropped(tmp_path: Path) -> None: - """When a generated_poc import line mixes known and unknown symbols, - the unknown symbol must still produce a flag_private finding (bug fix: - previously it was silently dropped from the report).""" + """When a generated_poc import line mixes known and unknown symbols, the + dry run reports the line once as a ``rename_import`` and the hint still + names the known symbol's ``adcp.types`` home (bug fix: the unknown symbol + used to drop the whole line from the report).""" _write( tmp_path, "code.py", @@ -1148,13 +1142,239 @@ def test_mixed_line_unknown_symbol_not_silently_dropped(tmp_path: Path) -> None: ) report = v3_to_v4.run(tmp_path, apply_changes=False) - all_private = [f for f in report.flagged if f.kind == "flag_private"] - # BrandReference: known symbol → per-symbol flag with after - known = [f for f in all_private if f.before == "BrandReference"] - assert len(known) == 1 - assert known[0].after == "adcp.types.BrandReference" + assert report.flagged == [] + (rename,) = [f for f in report.applied if f.kind == "rename_import"] + assert rename.before == "adcp.types.generated_poc.core.brand_ref" + assert rename.after == "adcp.types.domains.core.brand_ref" + assert "BrandReference → adcp.types.BrandReference" in (rename.hint or "") + + +# --------------------------------------------------------------------------- +# generated_poc → domains import-path rename (#1417) +# --------------------------------------------------------------------------- + +DEEP_IMPORTS = ( + "from adcp.types.generated_poc.media_buy.package_request import PackageRequest\n" + "from adcp.types.generated_poc.core.context import ContextObject\n" + "from adcp.types.generated_poc.brand import Brand, LocalizedName\n" + "from adcp.types.generated_poc.brand import Asset, AcquireRightsRequest\n" + "from adcp.types.generated_poc.brand.acquire_rights_request import " + "AcquireRightsRequest as ARR\n" + "from adcp.types.generated_poc.core.format import (\n" + " Assets81,\n" + ")\n" + "import adcp.types.generated_poc.core.error as err\n" + "import adcp.types.generated_poc.brand as brand_mod\n" + "\n" + "x = adcp.types.generated_poc.core.product.Product\n" + "y = brand_mod.Brand\n" +) + +DEEP_IMPORTS_RENAMED = ( + "from adcp.types.domains.media_buy.package_request import PackageRequest\n" + "from adcp.types.domains.core.context import ContextObject\n" + "from adcp.types.domains.brand_discovery import Brand, LocalizedName\n" + "from adcp.types.domains.brand_discovery import Asset\n" + "from adcp.types.domains.brand import AcquireRightsRequest\n" + "from adcp.types.domains.brand.acquire_rights_request import AcquireRightsRequest as ARR\n" + "from adcp.types.domains.core.format import (\n" + " Assets81,\n" + ")\n" + "import adcp.types.domains.core.error as err\n" + "import adcp.types.generated_poc.brand as brand_mod\n" + "\n" + "x = adcp.types.domains.core.product.Product\n" + "y = brand_mod.Brand\n" +) + + +def test_apply_renames_deep_generated_poc_imports_including_the_brand_split( + tmp_path: Path, +) -> None: + """The #1417 acceptance file: several deep imports, the ``brand`` split in + both directions, a mixed-half line, a multi-line import, a module import + and a dotted attribute access. Everything mechanical moves; the one + reference that cannot be placed — a bare ``generated_poc.brand`` module — + is flagged with both halves named.""" + path = _write(tmp_path, "app.py", DEEP_IMPORTS) + + report = v3_to_v4.run(tmp_path, apply_changes=True) + + assert path.read_text() == DEEP_IMPORTS_RENAMED + (split_flag,) = [f for f in report.flagged if f.kind == "flag_private"] + assert split_flag.line == 10 + assert split_flag.before == "adcp.types.generated_poc.brand" + assert split_flag.hint is not None + assert "adcp.types.domains.brand_discovery" in split_flag.hint + assert "adcp.types.domains.brand" in split_flag.hint + # The mixed-half line yields one rename per destination module. + line4 = sorted(f.after for f in report.applied if f.kind == "rename_import" and f.line == 4) + assert line4 == ["adcp.types.domains.brand", "adcp.types.domains.brand_discovery"] + assert path.read_text().count("\n") == DEEP_IMPORTS.count("\n") + 1 + + +def test_import_path_rename_is_idempotent(tmp_path: Path) -> None: + path = _write(tmp_path, "app.py", DEEP_IMPORTS) + v3_to_v4.run(tmp_path, apply_changes=True) + once = path.read_text() + + report = v3_to_v4.run(tmp_path, apply_changes=True) + + assert path.read_text() == once + assert [f for f in report.applied if f.kind == "rename_import"] == [] + + +def test_renamed_deep_imports_resolve_to_the_same_objects(tmp_path: Path) -> None: + """The rewrite is a rename, not a re-binding: every name the migrated file + imports is the object the deprecated path served.""" + import importlib + import warnings + + with warnings.catch_warnings(): + warnings.simplefilter("ignore", DeprecationWarning) + old_brand = importlib.import_module("adcp.types.generated_poc.brand") + old_context = importlib.import_module("adcp.types.generated_poc.core.context") + new_discovery = importlib.import_module("adcp.types.domains.brand_discovery") + new_brand = importlib.import_module("adcp.types.domains.brand") + new_context = importlib.import_module("adcp.types.domains.core.context") + + assert old_brand.Brand is new_discovery.Brand + assert old_brand.Asset is new_discovery.Asset # a name both halves bind: discovery wins + assert old_brand.AcquireRightsRequest is new_brand.AcquireRightsRequest + assert old_context.ContextObject is new_context.ContextObject + + +def test_dry_run_reports_import_path_renames_without_writing(tmp_path: Path) -> None: + path = _write(tmp_path, "app.py", DEEP_IMPORTS) + + report = v3_to_v4.run(tmp_path, apply_changes=False) + + assert path.read_text() == DEEP_IMPORTS + assert report.rewritten_files == 0 + renames = [f for f in report.applied if f.kind == "rename_import"] + assert len(renames) == 9 + assert all(f.after is not None and f.after.startswith("adcp.types.domains") for f in renames) + + +def test_import_path_rename_preserves_crlf(tmp_path: Path) -> None: + path = tmp_path / "code.py" + path.write_bytes( + b"from adcp.types.generated_poc.brand import Brand, AcquireRightsRequest\r\n" + b"from adcp.types.generated_poc.core.error import Error\r\n" + ) + + v3_to_v4.run(tmp_path, apply_changes=True) - # Unknown: unknown symbol → generic private-module flag (was silently - # dropped before this fix). - generic = [f for f in all_private if f.before == "adcp.types.generated_poc"] - assert len(generic) >= 1 + assert path.read_bytes() == ( + b"from adcp.types.domains.brand_discovery import Brand\r\n" + b"from adcp.types.domains.brand import AcquireRightsRequest\r\n" + b"from adcp.types.domains.core.error import Error\r\n" + ) + + +def test_split_stem_with_an_unknown_name_is_flagged_not_guessed(tmp_path: Path) -> None: + path = _write( + tmp_path, + "code.py", + "from adcp.types.generated_poc.brand import Brand, NoSuchThing\n", + ) + + report = v3_to_v4.run(tmp_path, apply_changes=True) + + assert path.read_text() == "from adcp.types.generated_poc.brand import Brand, NoSuchThing\n" + (flag,) = report.flagged + assert flag.kind == "flag_private" + assert flag.hint is not None and "NoSuchThing" in flag.hint + assert [f for f in report.applied if f.kind == "rename_import"] == [] + + +def test_split_stem_sharing_its_line_with_other_code_is_flagged(tmp_path: Path) -> None: + """Two statements are needed and a textual split would corrupt the line.""" + path = _write( + tmp_path, + "code.py", + "if True: from adcp.types.generated_poc.brand import Brand, AcquireRightsRequest\n", + ) + + report = v3_to_v4.run(tmp_path, apply_changes=True) + + assert "generated_poc" in path.read_text() + assert [f.kind for f in report.flagged] == ["flag_private"] + + +def test_auto_apply_lifts_to_the_flat_surface_before_renaming_the_rest(tmp_path: Path) -> None: + path = _write( + tmp_path, + "code.py", + "from adcp.types.generated_poc.core.context import ContextObject\n" + "from adcp.types.generated_poc.core.something import Unknown\n", + ) + + report = v3_to_v4.run(tmp_path, apply_changes=True, auto_apply=True) + + assert path.read_text() == ( + "from adcp.types import ContextObject\n" + "from adcp.types.domains.core.something import Unknown\n" + ) + assert [f.before for f in report.auto_applied] == ["ContextObject"] + assert [f.after for f in report.applied if f.kind == "rename_import"] == [ + "adcp.types.domains.core.something" + ] + + +def test_json_report_carries_import_path_renames_under_applied( + tmp_path: Path, capsys: pytest.CaptureFixture[str] +) -> None: + _write(tmp_path, "code.py", "from adcp.types.generated_poc.core.error import Error\n") + + v3_to_v4.main([str(tmp_path), "--json"]) + + payload = json.loads(capsys.readouterr().out) + kinds = {entry["kind"] for entry in payload["applied"]} + assert kinds == {"rename_import"} + assert payload["flagged"] == [] + + +def test_cli_exits_zero_when_only_import_paths_need_renaming(tmp_path: Path) -> None: + _write(tmp_path, "code.py", "from adcp.types.generated_poc.core.error import Error\n") + assert v3_to_v4.main([str(tmp_path)]) == 0 + + +def test_deprecation_warning_names_the_codemod_invocation_that_rewrites(tmp_path: Path) -> None: + """The warning, the docs and the codemod agree (#1417): the advice an + adopter reads names ``adcp migrate v3-to-v4 --apply``, and that invocation + performs the rewrite the advice promises.""" + import subprocess + import sys + + result = subprocess.run( + [sys.executable, "-W", "always", "-c", "import adcp.types.generated_poc.core.error"], + capture_output=True, + text=True, + timeout=300, + ) + assert result.returncode == 0, result.stderr + assert "`adcp migrate v3-to-v4 --apply` rewrites it" in result.stderr + + docs = Path(__file__).resolve().parents[1] / "docs" / "types-9-migration.md" + assert "`adcp migrate v3-to-v4 --apply` rewrites those lines" in docs.read_text("utf-8") + + _write(tmp_path, "code.py", "from adcp.types.generated_poc.core.error import Error\n") + assert v3_to_v4.main([str(tmp_path), "--apply", "--allow-dirty"]) == 0 + assert (tmp_path / "code.py").read_text() == "from adcp.types.domains.core.error import Error\n" + + +def test_split_detection_uses_the_same_suffix_the_runtime_alias_serves() -> None: + """The codemod cannot import the private alias module (import layering), so + it restates the one suffix the generator gives a root discovery schema; this + keeps the two spellings from drifting apart.""" + from adcp.types import _generated_poc_alias + + assert v3_to_v4.ROOT_DISCOVERY_SUFFIX == _generated_poc_alias.ROOT_DISCOVERY_SUFFIX + assert v3_to_v4.CANONICAL_TYPES_ROOT == _generated_poc_alias.CANONICAL_ROOT + assert v3_to_v4.DEPRECATED_TYPES_ROOT == _generated_poc_alias.DEPRECATED_ROOT + assert v3_to_v4._split_halves("brand") == ( + "adcp.types.domains.brand_discovery", + "adcp.types.domains.brand", + ) + assert v3_to_v4._split_halves("core") is None From eb9603561a8f022f055feb44f7326c35f63d7520 Mon Sep 17 00:00:00 2001 From: Brian O'Kelley Date: Tue, 6 Oct 2026 14:54:11 +0000 Subject: [PATCH 2/2] docs(migrate): note the dry-run exit code change; drop a redundant import A dry run whose only findings are deprecated generated_poc import paths now exits 0, since those are mechanical rename_import entries rather than flags. MIGRATION_v3_to_v4.md says so and tells a CI drift gate what to read instead. Co-Authored-By: Claude Fable 5.1 --- MIGRATION_v3_to_v4.md | 7 +++++++ tests/test_migrate_v3_to_v4.py | 1 - 2 files changed, 7 insertions(+), 1 deletion(-) diff --git a/MIGRATION_v3_to_v4.md b/MIGRATION_v3_to_v4.md index c0ca6f4ec..c35e7c4ff 100644 --- a/MIGRATION_v3_to_v4.md +++ b/MIGRATION_v3_to_v4.md @@ -36,6 +36,13 @@ imported from there instead of the domain path, and `Assets` numbered classes that have a documented semantic alias (`Assets81 → VideoFormatAsset`, etc.) are renamed. `flag_removed` findings always require human attention. +Since the `generated_poc` → `domains` rename landed (#1417), a dry run whose only +findings are deprecated `generated_poc` import paths exits 0: those are mechanical +`rename_import` entries under `applied`, no longer `flag_private` entries. A CI +drift gate that relied on the exit code to catch them should read `--json` and +fail on a non-empty `applied` list (or simply run `--apply` in CI and fail on a +dirty tree). + The CLI exits 0 when all remaining findings are mechanical (or none); exits 1 when `flag_removed` findings remain for human review — wire it into CI to gate merges until every flagged usage is addressed. diff --git a/tests/test_migrate_v3_to_v4.py b/tests/test_migrate_v3_to_v4.py index 78124be78..9ce9c5185 100644 --- a/tests/test_migrate_v3_to_v4.py +++ b/tests/test_migrate_v3_to_v4.py @@ -1227,7 +1227,6 @@ def test_import_path_rename_is_idempotent(tmp_path: Path) -> None: def test_renamed_deep_imports_resolve_to_the_same_objects(tmp_path: Path) -> None: """The rewrite is a rename, not a re-binding: every name the migrated file imports is the object the deprecated path served.""" - import importlib import warnings with warnings.catch_warnings():