From 21b9133c2875d7f0013fcc91178d149d65457e70 Mon Sep 17 00:00:00 2001 From: Max Ghenis Date: Mon, 12 Oct 2026 22:36:45 -0400 Subject: [PATCH 1/6] Check the measure ids the US legacy ETL cleanup doc names The cleanup doc's test checked only that the backticked package ids in its "Where the publisher's values live" tables are shipped packages. A renamed or mistyped measure id next to one passed CI, and two had drifted: - soi-congressional-district-2022 still named interest_paid_deduction_*, which chronicle#345 renamed to mortgage_interest_paid_* (N19300/A19300). - The SSI row put ssi_payments after three packages; it is a row of ssa-annual-statistical-supplement-2025's payment_amount, and the other two packages declare neither. The test now parses each "Chronicle package and measures" cell: an id belongs to every package listed since the previous id in its clause, must be a measure id or row value id that package declares (a prefix_* wildcard must match a measure), and an IRS column code must be a source column a matching measure reads. Unit tests cover the grammar with fake packages, including a nonexistent measure id, and a mutation of the real doc. It reads package YAML only and builds no facts. The doc fixes those two rows and the stale QBI note that the broad package's qbi_* read N03270/A03270 "on main" (chronicle#345 repinned them), worded as chronicle#350 words it. Co-Authored-By: Claude Opus 5.5 --- docs/us-legacy-etl-cleanup.md | 6 +- tests/test_chronicle_boundaries.py | 287 ++++++++++++++++++++++++++++- 2 files changed, 284 insertions(+), 9 deletions(-) diff --git a/docs/us-legacy-etl-cleanup.md b/docs/us-legacy-etl-cleanup.md index a7dea079..4edaf0c8 100644 --- a/docs/us-legacy-etl-cleanup.md +++ b/docs/us-legacy-etl-cleanup.md @@ -122,10 +122,10 @@ counted 32,067,810. The retired tests pinned the invented levels: for example | `db/etl_soi_credits.py` EITC by number of children (national) | `soi-table-2-5-eitc-agi-children-2022`, `soi-table-2-5-eitc-agi-children-2023`; by state: `soi-historic-table-2-state-eitc-2022` `eitc_no_children_*` through `eitc_three_or_more_children_*` | None. | | `db/etl_soi_credits.py` CTC and ACTC by number of children (national) | — | Gap: no packaged IRS table publishes CTC or ACTC by number of qualifying children. | | `db/etl_soi_deductions.py` `salt_claims`, `salt_amount` (state) | `soi-historic-table-2-state-broad-2022` `limited_state_local_taxes_*`, `real_estate_taxes_*` | The legacy "SALT" named no IRS line; HT2's total-taxes-paid column (`N18300`/`A18300`) is not packaged at state grain. | -| `db/etl_soi_deductions.py` `mortgage_interest_claims`, `mortgage_interest_amount` (state) | state: `soi-congressional-district-2022` `interest_paid_deduction_*` (`N19300`/`A19300`, state rows for all 51 geographies; chronicle#345 relabels the column as Schedule A line 8a home mortgage interest); national: `soi-table-2-1` `mortgage_interest_paid_*` | None for 2022. | +| `db/etl_soi_deductions.py` `mortgage_interest_claims`, `mortgage_interest_amount` (state) | state: `soi-congressional-district-2022` `mortgage_interest_paid_*` (`N19300`/`A19300`, Schedule A line 8a home mortgage interest, state rows for all 51 geographies); national: `soi-table-2-1` `mortgage_interest_paid_*` | None for 2022. | | `db/etl_soi_deductions.py` `charitable_claims`, `charitable_amount` (state) | state: `soi-congressional-district-2022` `charitable_*` (`N19700`/`A19700`, state rows for all 51 geographies); national: `soi-table-2-1` `charitable_*` | None for 2022. | | `db/etl_soi_deductions.py` `medical_claims`, `medical_amount` (state) | `soi-historic-table-2-state-broad-2022` `medical_dental_expense_*` | None for 2022. | -| `db/etl_soi_deductions.py` `qbi_claims`, `qbi_amount` (state) | `soi-congressional-district-2022` `qualified_business_income_deduction_*` (`N04475`/`A04475`, state rows for all 51 geographies); `soi-historic-table-2-state-broad-2022` `qbi_claims`, `qbi_amount` | None for 2022. The broad package's `qbi_*` read `N03270`/`A03270` on main; chronicle#345 repins them to `N04475`/`A04475`. | +| `db/etl_soi_deductions.py` `qbi_claims`, `qbi_amount` (state) | `soi-congressional-district-2022` `qualified_business_income_deduction_*` (`N04475`/`A04475`, state rows for all 51 geographies); `soi-historic-table-2-state-broad-2022` `qbi_claims`, `qbi_amount` | None for 2022. chronicle#345 repinned the broad package's `qbi_*` from `N03270`/`A03270` to `N04475`/`A04475`. | | `db/etl_soi_income_sources.py` taxable and tax-exempt interest, ordinary and qualified dividends, Schedule C, partnership and S corporation, rental and royalty (national and state) | national: `soi-historic-table-2` (US row); state: `soi-historic-table-2-state-broad-2022` `taxable_interest_*`, `tax_exempt_interest_*`, `ordinary_dividends_*`, `qualified_dividends_*`, `schedule_c_income_*`, `partnership_scorp_income_*`, `rental_royalty_income_*` | None for 2022. | | `db/etl_soi_income_sources.py` `short_term_capital_gains_*`, `long_term_capital_gains_*` | `soi-historic-table-2`, `soi-historic-table-2-state-broad-2022` and `soi-table-1-4` publish net capital gain (`net_capital_gains_*`) only | Gap: no packaged table splits gains into short and long term. | | `db/etl_soi_income_sources.py` `state_local_refunds_*` | — | Gap: HT2 publishes `N00700`/`A00700`, but no package selects it. | @@ -161,7 +161,7 @@ numbers with rounded, computed and invented ones. | `db/etl_medicaid.py` child and adult enrollment | `cms-medicaid-chip-monthly-enrollment-december-2024`, `cms-medicaid-chip-monthly-enrollment-dataset`: `medicaid_chip_child_enrollment`, `total_adult_medicaid_enrollment` by state | Earlier months are not packaged. | | `db/etl_medicaid.py` aged, disabled and expansion enrollment | — | Gap: no package carries these eligibility groups. | | `db/etl_ssa.py` OASDI beneficiaries, benefits and average benefits by type | `ssa-annual-statistical-supplement-2025` (calendar 2024 payments by program) | Gap: beneficiary counts and average benefits, and years before 2024. | -| `db/etl_ssa.py` and `db/etl_ssi.py` SSI recipients and payments, national and by state | `ssa-ssi-table-7b1-2024` (recipients and payments by state and category), `ssa-ssi-monthly-statistics-2024-12`, `ssa-annual-statistical-supplement-2025` `ssi_payments` | Years before 2024 are not packaged. | +| `db/etl_ssa.py` and `db/etl_ssi.py` SSI recipients and payments, national and by state | `ssa-ssi-table-7b1-2024` `recipient_count`, `payment_amount` (by state and category); `ssa-ssi-monthly-statistics-2024-12` `recipient_count`; `ssa-annual-statistical-supplement-2025` `payment_amount` (`ssi_payments` row) | Years before 2024 are not packaged. | | `db/etl_ssi.py` federal payment and state supplementation amounts | — | Gap: `ssa-ssi-monthly-statistics-2024-12` carries federal-payment recipients by age, not amounts. | | `db/etl_ssa.py` and `db/etl_ssi.py` `ssi_avg_monthly_payment` | — | Gap: no package carries average SSI payments. | | `db/etl_bls.py` and `db/etl_cps.py` labor force, employment, unemployment and their rates; median weekly earnings | — | Gap: no BLS package. | diff --git a/tests/test_chronicle_boundaries.py b/tests/test_chronicle_boundaries.py index 2b509cb0..2cccc033 100644 --- a/tests/test_chronicle_boundaries.py +++ b/tests/test_chronicle_boundaries.py @@ -661,6 +661,168 @@ def test_provenance_check_flags_rows_that_do_not_reproduce_their_cell( assert len(_legacy_rows_without_publisher_fact(mutated)) == expected +# The cleanup doc's "Where the publisher's values live" tables name, in this +# column, each package that replaces a retired series and then the ids it +# declares, for example `soi-historic-table-2-state-broad-2022` `ctc_*` +# (`N07225`/`A07225`). A cell splits into clauses at each `;` outside +# parentheses. Within a clause, an id belongs to every package listed since the +# previous id: `a`, `b`: `x` claims `x` for both packages, `a` `x`, `y` claims +# both ids for `a`. An exact id must be a measure id or a record-set row value +# id the package declares; a `prefix_*` wildcard must match at least one of its +# measure ids. An IRS column code names a source column that a measure matched +# by the id before it reads. +REPLACEMENT_COLUMN = "Chronicle package and measures" +DOC_PACKAGE_ID = re.compile(r"[a-z0-9]+(?:-[a-z0-9]+)+") +DOC_DECLARED_ID = re.compile(r"[a-z0-9][a-z0-9_]*(?:_\*)?") +DOC_COLUMN_CODE = re.compile(r"[AN][0-9]+") + +# measure id -> the source column ids it reads, and the row value ids. +Declarations = tuple[dict[str, set[str]], set[str]] + + +def _replacement_cells(doc: str) -> list[str]: + """Return each table row's "Chronicle package and measures" cell.""" + cells = [] + column = None + for line in doc.splitlines(): + if not line.startswith("|"): + column = None + continue + parts = [part.strip() for part in line.strip().strip("|").split("|")] + if column is None: + column = ( + parts.index(REPLACEMENT_COLUMN) if REPLACEMENT_COLUMN in parts else -1 + ) + elif column >= 0 and not set(line) <= set("|-: "): + cells.append(parts[column]) + return cells + + +def _clauses(cell: str) -> list[str]: + """Split a cell at each `;` outside parentheses.""" + clauses = [] + depth = start = 0 + for index, char in enumerate(cell): + depth += {"(": 1, ")": -1}.get(char, 0) + if char == ";" and depth == 0: + clauses.append(cell[start:index]) + start = index + 1 + clauses.append(cell[start:]) + return clauses + + +def _declarations(package) -> Declarations: + """Return a package's measure ids with their source columns, and its row ids.""" + measures: dict[str, set[str]] = {} + rows: set[str] = set() + for record_set in package.record_sets: + for measure in record_set.payload["measures"]: + columns = measures.setdefault(measure["measure_id"], set()) + if measure.get("source_column_id") is not None: + columns.add(str(measure["source_column_id"])) + rows.update(row["value_id"] for row in record_set.payload["rows"]) + return measures, rows + + +def _matching_measures(declared_id: str, measures: dict[str, set[str]]) -> list[str]: + if declared_id.endswith("_*"): + return [measure for measure in measures if measure.startswith(declared_id[:-1])] + return [declared_id] if declared_id in measures else [] + + +def _replacement_claim_errors( + cells: list[str], declarations: dict[str, Declarations] +) -> tuple[list[tuple[str, ...]], list[str]]: + """Check every package, id and column claim the cells make. + + ``declarations`` holds every shipped package the cells name. Returns the + claims checked and a message for each that the packages do not back. + """ + claims: list[tuple[str, ...]] = [] + errors: list[str] = [] + for cell in cells: + for clause in _clauses(cell): + packages: list[str] = [] + last_id = None + for token in re.findall(r"`([^`]+)`", clause): + if DOC_PACKAGE_ID.fullmatch(token): + if last_id is not None: + packages, last_id = [], None + packages.append(token) + if token not in declarations: + errors.append(f"`{token}` is not a shipped package") + elif DOC_COLUMN_CODE.fullmatch(token): + if last_id is None: + errors.append(f"{clause.strip()!r}: `{token}` follows no id") + continue + for package in packages: + if package not in declarations: + continue + measures = declarations[package][0] + claims.append((package, last_id, token)) + if not any( + token in measures[measure] + for measure in _matching_measures(last_id, measures) + ): + errors.append( + f"{package}: no `{last_id}` measure reads `{token}`" + ) + elif DOC_DECLARED_ID.fullmatch(token): + if not packages: + errors.append( + f"{clause.strip()!r}: `{token}` follows no package" + ) + for package in packages: + if package not in declarations: + continue + measures, rows = declarations[package] + claims.append((package, token)) + # Row ids never end in `_*`, so a wildcard matches + # measures only. + if ( + not _matching_measures(token, measures) + and token not in rows + ): + errors.append(f"{package}: declares no `{token}`") + last_id = token + else: + errors.append(f"{clause.strip()!r}: cannot read `{token}`") + return claims, errors + + +def _shipped_package_paths() -> dict[str, Path]: + """Map each package id under packages/ to its source_package.yaml.""" + return { + line.split(":", 1)[1].strip(): path + for path in (Path(__file__).resolve().parents[1] / "packages").rglob( + "source_package.yaml" + ) + for line in path.read_text().splitlines() + if line.startswith("package_id:") + } + + +def _us_legacy_etl_cleanup_doc_claim_errors( + doc: str, +) -> tuple[list[tuple[str, ...]], list[str]]: + """Check the doc's replacement claims against the packages they name.""" + from chronicle.source_package import load_source_package + + cells = _replacement_cells(doc) + paths = _shipped_package_paths() + named = { + token + for cell in cells + for token in re.findall(r"`([^`]+)`", cell) + if DOC_PACKAGE_ID.fullmatch(token) + } + declarations = { + package_id: _declarations(load_source_package(paths[package_id])) + for package_id in sorted(named & set(paths)) + } + return _replacement_claim_errors(cells, declarations) + + def test_us_legacy_etl_cleanup_doc_accounts_for_retired_series(): repo_root = Path(__file__).resolve().parents[1] doc = (repo_root / "docs" / "us-legacy-etl-cleanup.md").read_text() @@ -689,14 +851,127 @@ def test_us_legacy_etl_cleanup_doc_accounts_for_retired_series(): assert entry in doc # Every replacement the tables name is a package Chronicle ships. - package_ids = { - line.split(":", 1)[1].strip() - for path in (repo_root / "packages").rglob("source_package.yaml") - for line in path.read_text().splitlines() - if line.startswith("package_id:") - } + package_ids = set(_shipped_package_paths()) named = set( re.findall(r"`((?:soi|cms|census|ssa|cbo|usda|kff)-[a-z0-9-]+)`", doc) ) - set(RETIRED_US_LEGACY_ETLS.values()) assert named assert named <= package_ids + + # Every id a replacement cell names next to a package is one that package + # declares, and every IRS column code is one its measure reads. + cells = _replacement_cells(doc) + assert cells + assert all(cell == "—" or DOC_PACKAGE_ID.search(cell) for cell in cells) + claims, errors = _us_legacy_etl_cleanup_doc_claim_errors(doc) + assert errors == [] + assert {package for package, *_ in claims} >= { + "soi-historic-table-2-state-broad-2022", + "soi-congressional-district-2022", + "cms-aca-oep-state-level-2025", + "ssa-annual-statistical-supplement-2025", + } + + +# Packages for the replacement-claim check, as (measure id -> source columns, +# row value ids). +FAKE_DECLARATIONS: dict[str, Declarations] = { + "pkg-a": ( + { + "return_count": {"N1"}, + "ctc_claims": {"N07225"}, + "ctc_amount": {"A07225"}, + "payment_amount": set(), + }, + {"ssi_payments"}, + ), + "pkg-b": ({"return_count": {"N1"}}, set()), +} + + +def _fake_row_claim_errors(cell: str) -> tuple[list[tuple[str, ...]], list[str]]: + """Check one replacement cell, in a one-row table, against FAKE_DECLARATIONS.""" + doc = ( + f"| Series | {REPLACEMENT_COLUMN} | Gap |\n|---|---|---|\n| x | {cell} | y |\n" + ) + return _replacement_claim_errors(_replacement_cells(doc), FAKE_DECLARATIONS) + + +@pytest.mark.parametrize( + ("cell", "claims"), + [ + ("`pkg-a` `return_count`", [("pkg-a", "return_count")]), + ( + "`pkg-a` `ctc_*` (`N07225`/`A07225`, state rows; see note)", + [ + ("pkg-a", "ctc_*"), + ("pkg-a", "ctc_*", "N07225"), + ("pkg-a", "ctc_*", "A07225"), + ], + ), + ( + "`pkg-a` (2024), `pkg-b`: `return_count` by state", + [("pkg-a", "return_count"), ("pkg-b", "return_count")], + ), + ( + "`pkg-a` `ctc_claims` and `pkg-b` `return_count`; `pkg-b`", + [("pkg-a", "ctc_claims"), ("pkg-b", "return_count")], + ), + ( + "`pkg-a` `payment_amount` (`ssi_payments` row)", + [("pkg-a", "payment_amount"), ("pkg-a", "ssi_payments")], + ), + ("—", []), + ], +) +def test_replacement_claim_check_accepts_declared_ids(cell, claims): + assert _fake_row_claim_errors(cell) == (claims, []) + + +@pytest.mark.parametrize( + ("cell", "error"), + [ + ("`pkg-a` `return_counts`", "pkg-a: declares no `return_counts`"), + ("`pkg-a` `actc_*`", "pkg-a: declares no `actc_*`"), + ("`pkg-a` `ssi_*`", "pkg-a: declares no `ssi_*`"), + ("`pkg-a`, `pkg-b`: `ctc_claims`", "pkg-b: declares no `ctc_claims`"), + ("`pkg-a` `ctc_*` (`N11070`)", "pkg-a: no `ctc_*` measure reads `N11070`"), + ( + "`pkg-a` `ssi_payments` (`N1`)", + "pkg-a: no `ssi_payments` measure reads `N1`", + ), + ("`pkg-c` `return_count`", "`pkg-c` is not a shipped package"), + ( + "`return_count` in `pkg-a`", + "'`return_count` in `pkg-a`': `return_count` follows no package", + ), + ( + "`pkg-a`; `return_count`", + "'`return_count`': `return_count` follows no package", + ), + ("`pkg-a` (`N1`)", "'`pkg-a` (`N1`)': `N1` follows no id"), + ( + "`pkg-a` `Return count`", + "'`pkg-a` `Return count`': cannot read `Return count`", + ), + ], +) +def test_replacement_claim_check_flags_undeclared_ids(cell, error): + assert _fake_row_claim_errors(cell)[1] == [error] + + +def test_us_legacy_etl_cleanup_doc_check_flags_a_renamed_measure(): + """A measure id the doc names next to a real package must still exist.""" + doc = ( + Path(__file__).resolve().parents[1] / "docs" / "us-legacy-etl-cleanup.md" + ).read_text() + row = "`soi-historic-table-2-state-broad-2022` `return_count`, `adjusted_gross_income`" + assert row in doc + + _, errors = _us_legacy_etl_cleanup_doc_claim_errors( + doc.replace(row, row.replace("`return_count`", "`return_counts`")) + ) + + assert errors == [ + "soi-historic-table-2-state-broad-2022: declares no `return_counts`" + ] From 3e5071b49f93d31f2ece99c7ebde71c810599c0f Mon Sep 17 00:00:00 2001 From: Max Ghenis Date: Mon, 12 Oct 2026 22:36:55 -0400 Subject: [PATCH 2/6] Add changelog fragment for the cleanup doc measure-id check Co-Authored-By: Claude Opus 5.5 --- changelog.d/us-legacy-etl-doc-measure-ids.fixed.md | 6 ++++++ 1 file changed, 6 insertions(+) create mode 100644 changelog.d/us-legacy-etl-doc-measure-ids.fixed.md diff --git a/changelog.d/us-legacy-etl-doc-measure-ids.fixed.md b/changelog.d/us-legacy-etl-doc-measure-ids.fixed.md new file mode 100644 index 00000000..0356081f --- /dev/null +++ b/changelog.d/us-legacy-etl-doc-measure-ids.fixed.md @@ -0,0 +1,6 @@ +Check every measure id, row id and IRS column code the US legacy ETL cleanup +doc names next to a replacement package against that package's declarations, +and fix the two the check found: the congressional district file's line 8a +measures are `mortgage_interest_paid_*` since chronicle#345, and `ssi_payments` +is a row of the SSA supplement's `payment_amount`, not a measure of the SSI +Table 7.B1 or monthly statistics packages. From c0f750f9b05a5bcb18c8109585be910fb77dccaf Mon Sep 17 00:00:00 2001 From: Max Ghenis Date: Mon, 12 Oct 2026 22:38:53 -0400 Subject: [PATCH 3/6] Load each package's declarations once and check cells name a backticked package The renamed-measure mutation test reuses the declarations the doc test loaded, and the cell check looks for a backticked package id rather than any hyphenated word. Co-Authored-By: Claude Opus 5.5 --- tests/test_chronicle_boundaries.py | 17 +++++++++++------ 1 file changed, 11 insertions(+), 6 deletions(-) diff --git a/tests/test_chronicle_boundaries.py b/tests/test_chronicle_boundaries.py index 2cccc033..2548ca76 100644 --- a/tests/test_chronicle_boundaries.py +++ b/tests/test_chronicle_boundaries.py @@ -3,6 +3,7 @@ from __future__ import annotations import ast +import functools import hashlib import importlib.util import re @@ -711,11 +712,14 @@ def _clauses(cell: str) -> list[str]: return clauses -def _declarations(package) -> Declarations: +@functools.cache +def _declarations(package_path: Path) -> Declarations: """Return a package's measure ids with their source columns, and its row ids.""" + from chronicle.source_package import load_source_package + measures: dict[str, set[str]] = {} rows: set[str] = set() - for record_set in package.record_sets: + for record_set in load_source_package(package_path).record_sets: for measure in record_set.payload["measures"]: columns = measures.setdefault(measure["measure_id"], set()) if measure.get("source_column_id") is not None: @@ -725,6 +729,7 @@ def _declarations(package) -> Declarations: def _matching_measures(declared_id: str, measures: dict[str, set[str]]) -> list[str]: + """Return the measure ids an exact id or a `prefix_*` wildcard names.""" if declared_id.endswith("_*"): return [measure for measure in measures if measure.startswith(declared_id[:-1])] return [declared_id] if declared_id in measures else [] @@ -806,8 +811,6 @@ def _us_legacy_etl_cleanup_doc_claim_errors( doc: str, ) -> tuple[list[tuple[str, ...]], list[str]]: """Check the doc's replacement claims against the packages they name.""" - from chronicle.source_package import load_source_package - cells = _replacement_cells(doc) paths = _shipped_package_paths() named = { @@ -817,7 +820,7 @@ def _us_legacy_etl_cleanup_doc_claim_errors( if DOC_PACKAGE_ID.fullmatch(token) } declarations = { - package_id: _declarations(load_source_package(paths[package_id])) + package_id: _declarations(paths[package_id]) for package_id in sorted(named & set(paths)) } return _replacement_claim_errors(cells, declarations) @@ -862,7 +865,9 @@ def test_us_legacy_etl_cleanup_doc_accounts_for_retired_series(): # declares, and every IRS column code is one its measure reads. cells = _replacement_cells(doc) assert cells - assert all(cell == "—" or DOC_PACKAGE_ID.search(cell) for cell in cells) + for cell in cells: + tokens = re.findall(r"`([^`]+)`", cell) + assert cell == "—" or any(DOC_PACKAGE_ID.fullmatch(t) for t in tokens), cell claims, errors = _us_legacy_etl_cleanup_doc_claim_errors(doc) assert errors == [] assert {package for package, *_ in claims} >= { From 44f43f3828b84bd057daa86979378efe783fc056 Mon Sep 17 00:00:00 2001 From: Max Ghenis Date: Mon, 12 Oct 2026 23:27:39 -0400 Subject: [PATCH 4/6] Read the replacement tables as GitHub Markdown does Review of #354 found that valid Markdown slipped past the claim check. An indented row, or a row without outer pipes, ended the table early, so that row and the ones after it were never read. An escaped pipe split a cell, which dropped a wrong column code. A table now runs from its header to the next blank line. Rows may be indented or drop their outer pipes, and `\|` stays inside its cell. A row whose cell count differs from the header's fails. The doc test also requires one table per "Where the publisher's values live" section. The real-doc mutation test now covers each bypass the review reproduced; the old reader reported no error for any of them. The package-path scan is cached, and the grammar comment says that historical ids go without backticks. Co-Authored-By: Claude Opus 5.5 --- tests/test_chronicle_boundaries.py | 131 ++++++++++++++++++++++------- 1 file changed, 99 insertions(+), 32 deletions(-) diff --git a/tests/test_chronicle_boundaries.py b/tests/test_chronicle_boundaries.py index 2548ca76..5b6d5111 100644 --- a/tests/test_chronicle_boundaries.py +++ b/tests/test_chronicle_boundaries.py @@ -671,7 +671,8 @@ def test_provenance_check_flags_rows_that_do_not_reproduce_their_cell( # both ids for `a`. An exact id must be a measure id or a record-set row value # id the package declares; a `prefix_*` wildcard must match at least one of its # measure ids. An IRS column code names a source column that a measure matched -# by the id before it reads. +# by the id before it reads. Every backticked token in this column is a claim, +# so a historical id is written without backticks. REPLACEMENT_COLUMN = "Chronicle package and measures" DOC_PACKAGE_ID = re.compile(r"[a-z0-9]+(?:-[a-z0-9]+)+") DOC_DECLARED_ID = re.compile(r"[a-z0-9][a-z0-9_]*(?:_\*)?") @@ -681,22 +682,36 @@ def test_provenance_check_flags_rows_that_do_not_reproduce_their_cell( Declarations = tuple[dict[str, set[str]], set[str]] -def _replacement_cells(doc: str) -> list[str]: - """Return each table row's "Chronicle package and measures" cell.""" - cells = [] - column = None +def _table_cells(line: str) -> list[str]: + """Split a Markdown table row at its unescaped pipes; `\\|` is a cell's pipe.""" + row = line.strip().removeprefix("|") + if row.endswith("|") and not row.endswith("\\|"): + row = row[:-1] + return [cell.strip().replace("\\|", "|") for cell in re.split(r"(? list[list[str]]: + """Return the "Chronicle package and measures" cells of each table with one. + + As in GitHub Markdown, a table runs from its header row to the next blank + line, and a row may be indented or drop its outer pipes. Every row must have + the header's number of cells, so no row's claims are skipped or misread. + """ + tables: list[list[str]] = [] + header: list[str] | None = None for line in doc.splitlines(): - if not line.startswith("|"): - column = None - continue - parts = [part.strip() for part in line.strip().strip("|").split("|")] - if column is None: - column = ( - parts.index(REPLACEMENT_COLUMN) if REPLACEMENT_COLUMN in parts else -1 - ) - elif column >= 0 and not set(line) <= set("|-: "): - cells.append(parts[column]) - return cells + if not line.strip(): + header = None + elif header is None: + if "|" in line and REPLACEMENT_COLUMN in _table_cells(line): + header = _table_cells(line) + tables.append([]) + else: + cells = _table_cells(line) + assert len(cells) == len(header), f"{len(cells)} cells: {line}" + if not all(re.fullmatch(r":?-+:?", cell) for cell in cells): + tables[-1].append(cells[header.index(REPLACEMENT_COLUMN)]) + return tables def _clauses(cell: str) -> list[str]: @@ -795,6 +810,7 @@ def _replacement_claim_errors( return claims, errors +@functools.cache def _shipped_package_paths() -> dict[str, Path]: """Map each package id under packages/ to its source_package.yaml.""" return { @@ -811,7 +827,7 @@ def _us_legacy_etl_cleanup_doc_claim_errors( doc: str, ) -> tuple[list[tuple[str, ...]], list[str]]: """Check the doc's replacement claims against the packages they name.""" - cells = _replacement_cells(doc) + cells = [cell for table in _replacement_tables(doc) for cell in table] paths = _shipped_package_paths() named = { token @@ -862,10 +878,12 @@ def test_us_legacy_etl_cleanup_doc_accounts_for_retired_series(): assert named <= package_ids # Every id a replacement cell names next to a package is one that package - # declares, and every IRS column code is one its measure reads. - cells = _replacement_cells(doc) - assert cells - for cell in cells: + # declares, and every IRS column code is one its measure reads. Each + # "Where the publisher's values live" section has one replacement table. + tables = _replacement_tables(doc) + assert len(tables) == doc.count("### Where the publisher's values live") == 2 + assert all(tables) + for cell in (cell for table in tables for cell in table): tokens = re.findall(r"`([^`]+)`", cell) assert cell == "—" or any(DOC_PACKAGE_ID.fullmatch(t) for t in tokens), cell claims, errors = _us_legacy_etl_cleanup_doc_claim_errors(doc) @@ -899,7 +917,8 @@ def _fake_row_claim_errors(cell: str) -> tuple[list[tuple[str, ...]], list[str]] doc = ( f"| Series | {REPLACEMENT_COLUMN} | Gap |\n|---|---|---|\n| x | {cell} | y |\n" ) - return _replacement_claim_errors(_replacement_cells(doc), FAKE_DECLARATIONS) + (cells,) = _replacement_tables(doc) + return _replacement_claim_errors(cells, FAKE_DECLARATIONS) @pytest.mark.parametrize( @@ -965,18 +984,66 @@ def test_replacement_claim_check_flags_undeclared_ids(cell, error): assert _fake_row_claim_errors(cell)[1] == [error] -def test_us_legacy_etl_cleanup_doc_check_flags_a_renamed_measure(): - """A measure id the doc names next to a real package must still exist.""" +@pytest.mark.parametrize( + "row", + [ + "| x | `pkg-a` `return_count` | y | z |", + "| x | `pkg-a` `return_count`|`pkg-b` | y |", + "| x | `pkg-a` `return_count` |", + ], +) +def test_replacement_tables_reject_rows_off_the_header_width(row): + header = f"| Series | {REPLACEMENT_COLUMN} | Gap |\n|---|---|---|\n" + assert _replacement_tables(header + "| x | `pkg-a` `N1`\\|`A1` | y |\n") == [ + ["`pkg-a` `N1`|`A1`"] + ] + + with pytest.raises(AssertionError, match="cells: "): + _replacement_tables(header + row + "\n") + + +# Edits to the real doc, as (text, replacement, the one error they must raise). +# The indented row, the row without outer pipes and the escaped pipe are valid +# GitHub Markdown that a line-prefix or bare-pipe table reader skips or splits. +BROAD = "soi-historic-table-2-state-broad-2022" +DOC_MUTATIONS = { + "renamed measure": ( + f"`{BROAD}` `return_count`, `adjusted_gross_income`", + f"`{BROAD}` `return_counts`, `adjusted_gross_income`", + f"{BROAD}: declares no `return_counts`", + ), + "indented row with a mistyped wildcard": ( + f"| `db/etl_soi_deductions.py` `medical_claims`, `medical_amount` (state) " + f"| `{BROAD}` `medical_dental_expense_*`", + f" | `db/etl_soi_deductions.py` `medical_claims`, `medical_amount` (state) " + f"| `{BROAD}` `medical_dental_expens_*`", + f"{BROAD}: declares no `medical_dental_expens_*`", + ), + "row without outer pipes": ( + "| `db/etl_soi_deductions.py` `medical_claims`, `medical_amount` (state) " + f"| `{BROAD}` `medical_dental_expense_*` | None for 2022. |", + "`db/etl_soi_deductions.py` `medical_claims`, `medical_amount` (state) " + f"| `{BROAD}` `medical_dental_expense_amt` | None for 2022.", + f"{BROAD}: declares no `medical_dental_expense_amt`", + ), + "escaped pipe between column codes": ( + "`N19300`/`A19300`", + "`N19300`\\|`A19301`", + "soi-congressional-district-2022: no `mortgage_interest_paid_*` measure " + "reads `A19301`", + ), +} + + +@pytest.mark.parametrize("mutation", sorted(DOC_MUTATIONS)) +def test_us_legacy_etl_cleanup_doc_check_flags_edited_claims(mutation): + """A claim the doc's tables make next to a real package must still hold.""" doc = ( Path(__file__).resolve().parents[1] / "docs" / "us-legacy-etl-cleanup.md" ).read_text() - row = "`soi-historic-table-2-state-broad-2022` `return_count`, `adjusted_gross_income`" - assert row in doc + text, replacement, error = DOC_MUTATIONS[mutation] + assert doc.count(text) == 1 - _, errors = _us_legacy_etl_cleanup_doc_claim_errors( - doc.replace(row, row.replace("`return_count`", "`return_counts`")) - ) + _, errors = _us_legacy_etl_cleanup_doc_claim_errors(doc.replace(text, replacement)) - assert errors == [ - "soi-historic-table-2-state-broad-2022: declares no `return_counts`" - ] + assert errors == [error] From 309dc8a38be0202e959780a8fb8f4ed36c786d94 Mon Sep 17 00:00:00 2001 From: Max Ghenis Date: Mon, 12 Oct 2026 23:38:58 -0400 Subject: [PATCH 5/6] Find replacement tables by structure, not by exact header text The delta review of #354 found two more silent bypasses. - A repeated header written as **Chronicle package and measures** after a blank line started a second table, and the reader skipped its rows. - A header that named the column twice had only its first copy checked. Tables are now found as GitHub Markdown finds them: a row followed by a delimiter row with the same number of cells. Every column whose header, read without emphasis or code marks, is "Chronicle package and measures" is checked. Each "Where the publisher's values live" section must also hold exactly one table with the standard header. Both reproductions are real-doc mutation cases, and a mutation can now apply several edits. Co-Authored-By: Claude Opus 5.5 --- tests/test_chronicle_boundaries.py | 205 +++++++++++++++++++++-------- 1 file changed, 147 insertions(+), 58 deletions(-) diff --git a/tests/test_chronicle_boundaries.py b/tests/test_chronicle_boundaries.py index 5b6d5111..5c67fd8a 100644 --- a/tests/test_chronicle_boundaries.py +++ b/tests/test_chronicle_boundaries.py @@ -690,30 +690,56 @@ def _table_cells(line: str) -> list[str]: return [cell.strip().replace("\\|", "|") for cell in re.split(r"(? list[list[str]]: - """Return the "Chronicle package and measures" cells of each table with one. +def _is_delimiter_row(cells: list[str]) -> bool: + return all(re.fullmatch(r":?-+:?", cell) for cell in cells) - As in GitHub Markdown, a table runs from its header row to the next blank - line, and a row may be indented or drop its outer pipes. Every row must have - the header's number of cells, so no row's claims are skipped or misread. + +def _markdown_tables(doc: str) -> list[tuple[list[str], list[list[str]]]]: + """Return each table's header cells and body rows. + + As in GitHub Markdown, a table is a row followed by a delimiter row with as + many cells, and runs to the next blank line; a row may be indented or drop + its outer pipes. Every body row must have the header's number of cells, so + no row is skipped or misread. """ - tables: list[list[str]] = [] - header: list[str] | None = None - for line in doc.splitlines(): + tables: list[tuple[list[str], list[list[str]]]] = [] + lines = doc.splitlines() + rows: list[list[str]] | None = None + for index, line in enumerate(lines): if not line.strip(): - header = None - elif header is None: - if "|" in line and REPLACEMENT_COLUMN in _table_cells(line): - header = _table_cells(line) - tables.append([]) - else: + rows = None + elif rows is not None: cells = _table_cells(line) - assert len(cells) == len(header), f"{len(cells)} cells: {line}" - if not all(re.fullmatch(r":?-+:?", cell) for cell in cells): - tables[-1].append(cells[header.index(REPLACEMENT_COLUMN)]) + assert len(cells) == len(tables[-1][0]), f"{len(cells)} cells: {line}" + if not _is_delimiter_row(cells): + rows.append(cells) + elif "|" in line and index + 1 < len(lines): + header = _table_cells(line) + delimiter = _table_cells(lines[index + 1]) + if len(delimiter) == len(header) and _is_delimiter_row(delimiter): + rows = [] + tables.append((header, rows)) return tables +def _is_replacement_column(header_cell: str) -> bool: + """Whether a header cell, read without emphasis or code marks, is the column.""" + return re.sub(r"[*_`]", "", header_cell).strip().casefold() == ( + REPLACEMENT_COLUMN.casefold() + ) + + +def _replacement_cells(doc: str) -> list[str]: + """Return every cell of every "Chronicle package and measures" column.""" + return [ + row[column] + for header, rows in _markdown_tables(doc) + for row in rows + for column, cell in enumerate(header) + if _is_replacement_column(cell) + ] + + def _clauses(cell: str) -> list[str]: """Split a cell at each `;` outside parentheses.""" clauses = [] @@ -823,11 +849,33 @@ def _shipped_package_paths() -> dict[str, Path]: } +REPLACEMENT_SECTION = "Where the publisher's values live" +REPLACEMENT_HEADER = ["ETL series", REPLACEMENT_COLUMN, "Gap"] +LAYOUT_ERROR = ( + f"each {REPLACEMENT_SECTION!r} section must hold one table, headed " + + " | ".join(REPLACEMENT_HEADER) +) + + def _us_legacy_etl_cleanup_doc_claim_errors( doc: str, ) -> tuple[list[tuple[str, ...]], list[str]]: - """Check the doc's replacement claims against the packages they name.""" - cells = [cell for table in _replacement_tables(doc) for cell in table] + """Check the doc's replacement tables and the claims their cells make. + + Each of the two "Where the publisher's values live" sections must hold one + table, with the standard header. The claims of every replacement column in + the doc are checked, wherever its table is. + """ + headings = re.findall(r"^#+ (.*)$", doc, flags=re.MULTILINE) + sections = re.split(r"^#+ .*$", doc, flags=re.MULTILINE)[1:] + layout = [ + [header for header, _ in _markdown_tables(section)] + for heading, section in zip(headings, sections) + if heading == REPLACEMENT_SECTION + ] + layout_errors = [] if layout == [[REPLACEMENT_HEADER]] * 2 else [LAYOUT_ERROR] + + cells = _replacement_cells(doc) paths = _shipped_package_paths() named = { token @@ -839,7 +887,8 @@ def _us_legacy_etl_cleanup_doc_claim_errors( package_id: _declarations(paths[package_id]) for package_id in sorted(named & set(paths)) } - return _replacement_claim_errors(cells, declarations) + claims, errors = _replacement_claim_errors(cells, declarations) + return claims, layout_errors + errors def test_us_legacy_etl_cleanup_doc_accounts_for_retired_series(): @@ -877,13 +926,12 @@ def test_us_legacy_etl_cleanup_doc_accounts_for_retired_series(): assert named assert named <= package_ids - # Every id a replacement cell names next to a package is one that package - # declares, and every IRS column code is one its measure reads. Each - # "Where the publisher's values live" section has one replacement table. - tables = _replacement_tables(doc) - assert len(tables) == doc.count("### Where the publisher's values live") == 2 - assert all(tables) - for cell in (cell for table in tables for cell in table): + # Each "Where the publisher's values live" section holds one replacement + # table. Every id a replacement cell names next to a package is one that + # package declares, and every IRS column code is one its measure reads. + cells = _replacement_cells(doc) + assert cells + for cell in cells: tokens = re.findall(r"`([^`]+)`", cell) assert cell == "—" or any(DOC_PACKAGE_ID.fullmatch(t) for t in tokens), cell claims, errors = _us_legacy_etl_cleanup_doc_claim_errors(doc) @@ -917,8 +965,7 @@ def _fake_row_claim_errors(cell: str) -> tuple[list[tuple[str, ...]], list[str]] doc = ( f"| Series | {REPLACEMENT_COLUMN} | Gap |\n|---|---|---|\n| x | {cell} | y |\n" ) - (cells,) = _replacement_tables(doc) - return _replacement_claim_errors(cells, FAKE_DECLARATIONS) + return _replacement_claim_errors(_replacement_cells(doc), FAKE_DECLARATIONS) @pytest.mark.parametrize( @@ -992,45 +1039,87 @@ def test_replacement_claim_check_flags_undeclared_ids(cell, error): "| x | `pkg-a` `return_count` |", ], ) -def test_replacement_tables_reject_rows_off_the_header_width(row): +def test_markdown_tables_reject_rows_off_the_header_width(row): header = f"| Series | {REPLACEMENT_COLUMN} | Gap |\n|---|---|---|\n" - assert _replacement_tables(header + "| x | `pkg-a` `N1`\\|`A1` | y |\n") == [ - ["`pkg-a` `N1`|`A1`"] + assert _replacement_cells(header + "| x | `pkg-a` `N1`\\|`A1` | y |\n") == [ + "`pkg-a` `N1`|`A1`" ] with pytest.raises(AssertionError, match="cells: "): - _replacement_tables(header + row + "\n") + _markdown_tables(header + row + "\n") + + +def test_replacement_cells_read_every_replacement_column(): + doc = ( + "Prose | with a pipe\n\n" + f"| Series | **{REPLACEMENT_COLUMN}** | `{REPLACEMENT_COLUMN.lower()}` |\n" + "| :--- | --- | ---: |\n" + "| x | `pkg-a` | `pkg-b` |\n\n" + "| Series | Chronicle packages |\n|---|---|\n| x | `pkg-c` |\n" + ) + assert _replacement_cells(doc) == ["`pkg-a`", "`pkg-b`"] -# Edits to the real doc, as (text, replacement, the one error they must raise). -# The indented row, the row without outer pipes and the escaped pipe are valid -# GitHub Markdown that a line-prefix or bare-pipe table reader skips or splits. + +# Edits to the real doc, as ((text, replacement) pairs, the errors they must +# raise). The indented row, the row without outer pipes, the escaped pipe, the +# second table with a bold header and the repeated column are valid GitHub +# Markdown that a line-prefix, bare-pipe or exact-header table reader skips or +# splits. BROAD = "soi-historic-table-2-state-broad-2022" +MEDICAL_ROW = ( + "| `db/etl_soi_deductions.py` `medical_claims`, `medical_amount` (state) " + f"| `{BROAD}` `medical_dental_expense_*` | None for 2022. |" +) +ACA_HEADER = ( + f"| {' | '.join(REPLACEMENT_HEADER)} |\n|---|---|---|\n" + "| `db/etl_aca_enrollment.py` state and national plan selections" +) +ACA_GAP = ( + "| The OEP packages publish state rows only, so national totals are a gap, " + "and the 2023 OEP file is not packaged. |" +) DOC_MUTATIONS = { "renamed measure": ( - f"`{BROAD}` `return_count`, `adjusted_gross_income`", - f"`{BROAD}` `return_counts`, `adjusted_gross_income`", - f"{BROAD}: declares no `return_counts`", + [ + ( + f"`{BROAD}` `return_count`, `adjusted_gross_income`", + f"`{BROAD}` `return_counts`, `adjusted_gross_income`", + ) + ], + [f"{BROAD}: declares no `return_counts`"], ), "indented row with a mistyped wildcard": ( - f"| `db/etl_soi_deductions.py` `medical_claims`, `medical_amount` (state) " - f"| `{BROAD}` `medical_dental_expense_*`", - f" | `db/etl_soi_deductions.py` `medical_claims`, `medical_amount` (state) " - f"| `{BROAD}` `medical_dental_expens_*`", - f"{BROAD}: declares no `medical_dental_expens_*`", + [(MEDICAL_ROW, " " + MEDICAL_ROW.replace("expense_*", "expens_*"))], + [f"{BROAD}: declares no `medical_dental_expens_*`"], ), "row without outer pipes": ( - "| `db/etl_soi_deductions.py` `medical_claims`, `medical_amount` (state) " - f"| `{BROAD}` `medical_dental_expense_*` | None for 2022. |", - "`db/etl_soi_deductions.py` `medical_claims`, `medical_amount` (state) " - f"| `{BROAD}` `medical_dental_expense_amt` | None for 2022.", - f"{BROAD}: declares no `medical_dental_expense_amt`", + [(MEDICAL_ROW, MEDICAL_ROW.strip("|").replace("expense_*", "expense_amt"))], + [f"{BROAD}: declares no `medical_dental_expense_amt`"], ), "escaped pipe between column codes": ( - "`N19300`/`A19300`", - "`N19300`\\|`A19301`", - "soi-congressional-district-2022: no `mortgage_interest_paid_*` measure " - "reads `A19301`", + [("`N19300`/`A19300`", "`N19300`\\|`A19301`")], + [ + "soi-congressional-district-2022: no `mortgage_interest_paid_*` " + "measure reads `A19301`" + ], + ), + "second table with a bold header": ( + [ + ( + MEDICAL_ROW, + f"\n| ETL series | **{REPLACEMENT_COLUMN}** | Gap |\n|---|---|---|\n" + + MEDICAL_ROW.replace("expense_*", "expens_*"), + ) + ], + [LAYOUT_ERROR, f"{BROAD}: declares no `medical_dental_expens_*`"], + ), + "repeated replacement column": ( + [ + (ACA_HEADER, ACA_HEADER.replace("| Gap |", f"| {REPLACEMENT_COLUMN} |")), + (ACA_GAP, f"| `{BROAD}` `medical_dental_expense_amt` |"), + ], + [LAYOUT_ERROR, f"{BROAD}: declares no `medical_dental_expense_amt`"], ), } @@ -1041,9 +1130,9 @@ def test_us_legacy_etl_cleanup_doc_check_flags_edited_claims(mutation): doc = ( Path(__file__).resolve().parents[1] / "docs" / "us-legacy-etl-cleanup.md" ).read_text() - text, replacement, error = DOC_MUTATIONS[mutation] - assert doc.count(text) == 1 - - _, errors = _us_legacy_etl_cleanup_doc_claim_errors(doc.replace(text, replacement)) + edits, expected = DOC_MUTATIONS[mutation] + for text, replacement in edits: + assert doc.count(text) == 1 + doc = doc.replace(text, replacement) - assert errors == [error] + assert _us_legacy_etl_cleanup_doc_claim_errors(doc)[1] == expected From ebffab44d6eea061925f565893555563412e88bd Mon Sep 17 00:00:00 2001 From: Max Ghenis Date: Mon, 12 Oct 2026 23:51:10 -0400 Subject: [PATCH 6/6] Read the cleanup doc's tables with a GFM parser and pin its layout The third review of #354 found that a replacement table inside a blockquote, list item or
block was skipped. It also found that valid heading forms (indented, closing hashes, setext) raised a false layout error, and that an extra table headed with an escaped asterisk went unread. The test now reads the doc with markdown-it-py (CommonMark plus GitHub's table extension), so a table counts wherever GitHub renders one, under a heading of any form. Cells are read as GitHub reads them: `\|` is a pipe, short rows are padded and extra cells are dropped. DOC_TABLES pins every table's heading and header. A table added, moved or reheaded fails until it is listed, and the replacement-column check still reads it in the meantime. markdown-it-py was already locked at 4.0.0 through rich; it is now a declared dev dependency, and the lock gains metadata only. Every reproduction from the three reviews is a real-doc mutation case, and reformatted headings are a passing case. Co-Authored-By: Claude Opus 5.5 --- pyproject.toml | 2 + tests/test_chronicle_boundaries.py | 237 +++++++++++++++++++---------- uv.lock | 8 +- 3 files changed, 162 insertions(+), 85 deletions(-) diff --git a/pyproject.toml b/pyproject.toml index 320543ab..3f005afa 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -26,6 +26,7 @@ dependencies = [ [project.optional-dependencies] dev = [ + "markdown-it-py>=3.0.0", # Doc-table tests read Markdown as GitHub does "pytest>=8.0.0,<9", "ruff>=0.5.0", ] @@ -58,5 +59,6 @@ target-version = "py311" [dependency-groups] dev = [ + "markdown-it-py>=3.0.0", "pytest>=8.0.0,<9", ] diff --git a/tests/test_chronicle_boundaries.py b/tests/test_chronicle_boundaries.py index 5c67fd8a..406fe113 100644 --- a/tests/test_chronicle_boundaries.py +++ b/tests/test_chronicle_boundaries.py @@ -12,6 +12,7 @@ import pytest import yaml +from markdown_it import MarkdownIt FORBIDDEN_PACKAGE_ROOTS = {"calibration", "micro"} @@ -682,49 +683,42 @@ def test_provenance_check_flags_rows_that_do_not_reproduce_their_cell( Declarations = tuple[dict[str, set[str]], set[str]] -def _table_cells(line: str) -> list[str]: - """Split a Markdown table row at its unescaped pipes; `\\|` is a cell's pipe.""" - row = line.strip().removeprefix("|") - if row.endswith("|") and not row.endswith("\\|"): - row = row[:-1] - return [cell.strip().replace("\\|", "|") for cell in re.split(r"(? list[tuple[str, list[str], list[list[str]]]]: + """Return each table as (the heading above it, header cells, body rows). - -def _is_delimiter_row(cells: list[str]) -> bool: - return all(re.fullmatch(r":?-+:?", cell) for cell in cells) - - -def _markdown_tables(doc: str) -> list[tuple[list[str], list[list[str]]]]: - """Return each table's header cells and body rows. - - As in GitHub Markdown, a table is a row followed by a delimiter row with as - many cells, and runs to the next blank line; a row may be indented or drop - its outer pipes. Every body row must have the header's number of cells, so - no row is skipped or misread. + The doc is read by a CommonMark parser with GitHub's table extension, so a + table counts wherever GitHub renders one: indented, without outer pipes, in + a blockquote, list item or HTML block, under a heading of any form. A cell + is its Markdown source, with `\\|` read as a pipe; a short row is padded. """ - tables: list[tuple[list[str], list[list[str]]]] = [] - lines = doc.splitlines() - rows: list[list[str]] | None = None - for index, line in enumerate(lines): - if not line.strip(): - rows = None - elif rows is not None: - cells = _table_cells(line) - assert len(cells) == len(tables[-1][0]), f"{len(cells)} cells: {line}" - if not _is_delimiter_row(cells): - rows.append(cells) - elif "|" in line and index + 1 < len(lines): - header = _table_cells(line) - delimiter = _table_cells(lines[index + 1]) - if len(delimiter) == len(header) and _is_delimiter_row(delimiter): - rows = [] - tables.append((header, rows)) + tables: list[tuple[str, list[str], list[list[str]]]] = [] + heading = "" + in_heading = in_header = in_cell = False + row: list[str] = [] + for token in MarkdownIt("commonmark", {"html": True}).enable("table").parse(doc): + kind = token.type + if kind in ("heading_open", "heading_close"): + in_heading = kind == "heading_open" + elif kind in ("thead_open", "thead_close"): + in_header = kind == "thead_open" + if in_header: + tables.append((heading, [], [])) + elif kind == "tr_open": + row = tables[-1][1] if in_header else [] + if not in_header: + tables[-1][2].append(row) + elif kind in ("th_open", "td_open", "th_close", "td_close"): + in_cell = kind.endswith("_open") + elif kind == "inline" and in_heading: + heading = token.content + elif kind == "inline" and in_cell: + row.append(token.content) return tables def _is_replacement_column(header_cell: str) -> bool: - """Whether a header cell, read without emphasis or code marks, is the column.""" - return re.sub(r"[*_`]", "", header_cell).strip().casefold() == ( + """Whether a header cell, read without emphasis, code or escape marks, is it.""" + return re.sub(r"[\\*_`]", "", header_cell).strip().casefold() == ( REPLACEMENT_COLUMN.casefold() ) @@ -733,7 +727,7 @@ def _replacement_cells(doc: str) -> list[str]: """Return every cell of every "Chronicle package and measures" column.""" return [ row[column] - for header, rows in _markdown_tables(doc) + for _, header, rows in _markdown_tables(doc) for row in rows for column, cell in enumerate(header) if _is_replacement_column(cell) @@ -851,29 +845,45 @@ def _shipped_package_paths() -> dict[str, Path]: REPLACEMENT_SECTION = "Where the publisher's values live" REPLACEMENT_HEADER = ["ETL series", REPLACEMENT_COLUMN, "Gap"] -LAYOUT_ERROR = ( - f"each {REPLACEMENT_SECTION!r} section must hold one table, headed " - + " | ".join(REPLACEMENT_HEADER) -) +# Every table in the cleanup doc, as (heading above it, header). A table added, +# moved or reheaded fails the doc test until it is listed here, so no table can +# make replacement claims the check does not read. +DOC_TABLES = [ + ("US legacy ETL cleanup", ["Loader", "Outcome", "Why"]), + ( + "Retired: invented IRS SOI state values", + ["Module", "What its comments said", "Rows it wrote"], + ), + ( + "Evidence", + [ + "Legacy series (2021, 51 geographies)", + "IRS column", + "Within 0.5%", + "Median error", + "Example, legacy vs IRS", + ], + ), + (REPLACEMENT_SECTION, REPLACEMENT_HEADER), + ( + "Retired: hand-typed values from other publishers", + ["Module", "Rows on main", "Evidence"], + ), + (REPLACEMENT_SECTION, REPLACEMENT_HEADER), +] +LAYOUT_ERROR = "the cleanup doc's tables differ from DOC_TABLES" def _us_legacy_etl_cleanup_doc_claim_errors( doc: str, ) -> tuple[list[tuple[str, ...]], list[str]]: - """Check the doc's replacement tables and the claims their cells make. + """Check the doc's table layout and the claims its replacement cells make. - Each of the two "Where the publisher's values live" sections must hold one - table, with the standard header. The claims of every replacement column in - the doc are checked, wherever its table is. + The doc's tables must be DOC_TABLES. The claims of every replacement column + are checked, wherever its table is. """ - headings = re.findall(r"^#+ (.*)$", doc, flags=re.MULTILINE) - sections = re.split(r"^#+ .*$", doc, flags=re.MULTILINE)[1:] - layout = [ - [header for header, _ in _markdown_tables(section)] - for heading, section in zip(headings, sections) - if heading == REPLACEMENT_SECTION - ] - layout_errors = [] if layout == [[REPLACEMENT_HEADER]] * 2 else [LAYOUT_ERROR] + layout = [(heading, header) for heading, header, _ in _markdown_tables(doc)] + layout_errors = [] if layout == DOC_TABLES else [LAYOUT_ERROR] cells = _replacement_cells(doc) paths = _shipped_package_paths() @@ -1031,46 +1041,51 @@ def test_replacement_claim_check_flags_undeclared_ids(cell, error): assert _fake_row_claim_errors(cell)[1] == [error] -@pytest.mark.parametrize( - "row", - [ - "| x | `pkg-a` `return_count` | y | z |", - "| x | `pkg-a` `return_count`|`pkg-b` | y |", - "| x | `pkg-a` `return_count` |", - ], -) -def test_markdown_tables_reject_rows_off_the_header_width(row): - header = f"| Series | {REPLACEMENT_COLUMN} | Gap |\n|---|---|---|\n" - assert _replacement_cells(header + "| x | `pkg-a` `N1`\\|`A1` | y |\n") == [ - "`pkg-a` `N1`|`A1`" - ] +def test_markdown_tables_read_cells_as_github_renders_them(): + doc = ( + "## Heading ##\n\nProse | with a pipe\n\n" + f"| Series | {REPLACEMENT_COLUMN} | Gap |\n|---|---|---|\n" + "| x | `pkg-a` `N1`\\|`A1` | y | not rendered |\n" + " w | `pkg-b`\n" + ) - with pytest.raises(AssertionError, match="cells: "): - _markdown_tables(header + row + "\n") + assert _markdown_tables(doc) == [ + ( + "Heading", + ["Series", REPLACEMENT_COLUMN, "Gap"], + [["x", "`pkg-a` `N1`|`A1`", "y"], ["w", "`pkg-b`", ""]], + ) + ] def test_replacement_cells_read_every_replacement_column(): doc = ( - "Prose | with a pipe\n\n" f"| Series | **{REPLACEMENT_COLUMN}** | `{REPLACEMENT_COLUMN.lower()}` |\n" "| :--- | --- | ---: |\n" "| x | `pkg-a` | `pkg-b` |\n\n" - "| Series | Chronicle packages |\n|---|---|\n| x | `pkg-c` |\n" + f"> | Series | {REPLACEMENT_COLUMN}\\* |\n> |---|---|\n> | x | `pkg-c` |\n\n" + "| Series | Chronicle packages |\n|---|---|\n| x | `pkg-d` |\n" ) - assert _replacement_cells(doc) == ["`pkg-a`", "`pkg-b`"] + assert _replacement_cells(doc) == ["`pkg-a`", "`pkg-b`", "`pkg-c`"] # Edits to the real doc, as ((text, replacement) pairs, the errors they must -# raise). The indented row, the row without outer pipes, the escaped pipe, the -# second table with a bold header and the repeated column are valid GitHub -# Markdown that a line-prefix, bare-pipe or exact-header table reader skips or -# splits. +# raise). Each is valid GitHub Markdown that shows the edited claim in a +# rendered replacement column; the indented row, row without outer pipes, +# escaped pipe, bold or escaped header, repeated column and tables in a +# blockquote, list item or HTML block each got past an earlier table reader. BROAD = "soi-historic-table-2-state-broad-2022" MEDICAL_ROW = ( "| `db/etl_soi_deductions.py` `medical_claims`, `medical_amount` (state) " f"| `{BROAD}` `medical_dental_expense_*` | None for 2022. |" ) +BAD_MEDICAL_ROW = MEDICAL_ROW.replace("expense_*", "expense_amt") +BAD_MEDICAL_ERROR = f"{BROAD}: declares no `medical_dental_expense_amt`" +BAD_MEDICAL_TABLE = ( + f"| {' | '.join(REPLACEMENT_HEADER)} |\n|---|---|---|\n{BAD_MEDICAL_ROW}" +) +OTHER_PUBLISHERS = "## Retired: hand-typed values from other publishers" ACA_HEADER = ( f"| {' | '.join(REPLACEMENT_HEADER)} |\n|---|---|---|\n" "| `db/etl_aca_enrollment.py` state and national plan selections" @@ -1079,6 +1094,16 @@ def test_replacement_cells_read_every_replacement_column(): "| The OEP packages publish state rows only, so national totals are a gap, " "and the 2023 OEP file is not packaged. |" ) + + +def _move_bad_medical_row(table: str) -> list[tuple[str, str]]: + """Edits that move the medical row, mistyped, into `table` further down.""" + return [ + (MEDICAL_ROW + "\n", ""), + (OTHER_PUBLISHERS, f"{table}\n\n{OTHER_PUBLISHERS}"), + ] + + DOC_MUTATIONS = { "renamed measure": ( [ @@ -1089,13 +1114,13 @@ def test_replacement_cells_read_every_replacement_column(): ], [f"{BROAD}: declares no `return_counts`"], ), - "indented row with a mistyped wildcard": ( - [(MEDICAL_ROW, " " + MEDICAL_ROW.replace("expense_*", "expens_*"))], - [f"{BROAD}: declares no `medical_dental_expens_*`"], + "indented row": ( + [(MEDICAL_ROW, " " + BAD_MEDICAL_ROW)], + [BAD_MEDICAL_ERROR], ), "row without outer pipes": ( - [(MEDICAL_ROW, MEDICAL_ROW.strip("|").replace("expense_*", "expense_amt"))], - [f"{BROAD}: declares no `medical_dental_expense_amt`"], + [(MEDICAL_ROW, BAD_MEDICAL_ROW.strip("|"))], + [BAD_MEDICAL_ERROR], ), "escaped pipe between column codes": ( [("`N19300`/`A19300`", "`N19300`\\|`A19301`")], @@ -1109,17 +1134,38 @@ def test_replacement_cells_read_every_replacement_column(): ( MEDICAL_ROW, f"\n| ETL series | **{REPLACEMENT_COLUMN}** | Gap |\n|---|---|---|\n" - + MEDICAL_ROW.replace("expense_*", "expens_*"), + + BAD_MEDICAL_ROW, ) ], - [LAYOUT_ERROR, f"{BROAD}: declares no `medical_dental_expens_*`"], + [LAYOUT_ERROR, BAD_MEDICAL_ERROR], ), "repeated replacement column": ( [ (ACA_HEADER, ACA_HEADER.replace("| Gap |", f"| {REPLACEMENT_COLUMN} |")), (ACA_GAP, f"| `{BROAD}` `medical_dental_expense_amt` |"), ], - [LAYOUT_ERROR, f"{BROAD}: declares no `medical_dental_expense_amt`"], + [LAYOUT_ERROR, BAD_MEDICAL_ERROR], + ), + "row moved into a blockquote": ( + _move_bad_medical_row( + "\n".join(f"> {line}" for line in BAD_MEDICAL_TABLE.splitlines()) + ), + [LAYOUT_ERROR, BAD_MEDICAL_ERROR], + ), + "row moved into a list item": ( + _move_bad_medical_row("- " + BAD_MEDICAL_TABLE.replace("\n", "\n ")), + [LAYOUT_ERROR, BAD_MEDICAL_ERROR], + ), + "row moved into an HTML block": ( + _move_bad_medical_row(f"
\n\n{BAD_MEDICAL_TABLE}\n\n
"), + [LAYOUT_ERROR, BAD_MEDICAL_ERROR], + ), + "row moved to a new table with an escaped header": ( + _move_bad_medical_row( + "### More replacements\n\n" + + BAD_MEDICAL_TABLE.replace(REPLACEMENT_COLUMN, REPLACEMENT_COLUMN + "\\*") + ), + [LAYOUT_ERROR, BAD_MEDICAL_ERROR], ), } @@ -1136,3 +1182,26 @@ def test_us_legacy_etl_cleanup_doc_check_flags_edited_claims(mutation): doc = doc.replace(text, replacement) assert _us_legacy_etl_cleanup_doc_claim_errors(doc)[1] == expected + + +@pytest.mark.parametrize( + "heading", + [ + " ### Where the publisher's values live ### ", + "Where the publisher's values live\n---", + "Where the publisher's values live\n===", + ], +) +def test_us_legacy_etl_cleanup_doc_check_reads_any_heading_form(heading): + doc = ( + Path(__file__).resolve().parents[1] / "docs" / "us-legacy-etl-cleanup.md" + ).read_text() + original = "### Where the publisher's values live" + assert doc.count(original) == 2 + + claims, errors = _us_legacy_etl_cleanup_doc_claim_errors( + doc.replace(original, heading) + ) + + assert errors == [] + assert claims diff --git a/uv.lock b/uv.lock index 44cca7aa..48b77eb3 100644 --- a/uv.lock +++ b/uv.lock @@ -907,12 +907,14 @@ dependencies = [ [package.optional-dependencies] dev = [ + { name = "markdown-it-py" }, { name = "pytest" }, { name = "ruff" }, ] [package.dev-dependencies] dev = [ + { name = "markdown-it-py" }, { name = "pytest" }, ] @@ -920,6 +922,7 @@ dev = [ requires-dist = [ { name = "httpx", specifier = ">=0.27.0" }, { name = "jsonschema", specifier = ">=4.20.0" }, + { name = "markdown-it-py", marker = "extra == 'dev'", specifier = ">=3.0.0" }, { name = "numpy", specifier = ">=1.24.0" }, { name = "odfpy", specifier = ">=1.4.1" }, { name = "openpyxl", specifier = ">=3.1.0" }, @@ -936,7 +939,10 @@ requires-dist = [ provides-extras = ["dev"] [package.metadata.requires-dev] -dev = [{ name = "pytest", specifier = ">=8.0.0,<9" }] +dev = [ + { name = "markdown-it-py", specifier = ">=3.0.0" }, + { name = "pytest", specifier = ">=8.0.0,<9" }, +] [[package]] name = "postgrest"