PolicyEngine / PolicyEngine/chronicle

Collapse source_package.py's parser dispatch chains into a lane registry

Open
#169 1 comment 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

Dominant language
Python
Stars
0
Forks
1
Avg merge
11h 48m
Merged PRs (30d)
45

Description

Problem

chronicle/source_package.py dispatches parsers through per-lane if self.parser == ...
stanzas duplicated across build_source_rows and build_source_cells. The chain was ~10
branches when first raised in review on #141; after #163 it is 26+, growing roughly one
lane per wave. Three costs:

Every lane registers twice, and forgetting has an asymmetric failure. Each full-row
parser needs a stanza in both methods. Missing from build_source_cells fails loudly
(ValueError: Unsupported source artifact parser). Missing from build_source_rows hits
the return [] fallthrough — facts still emit (the cells path re-parses rows itself), but
the preserved source-row table comes out silently empty. The fallthrough cannot simply
raise, because return [] is legitimate for cell-only lanes (xlsx_used_range and
friends) that have no row representation: the structure cannot distinguish "this lane has
no rows half" from "someone forgot to wire the rows half".

A ~20-line stanza is copy-pasted twelve times. Every full-row branch in
build_source_cells is identical except the parser-function name: rebuild rows if not
passed, render selected_rows through _render_value, call
source_cells_from_source_rows. Any change to that shared logic must be applied twelve
times or it drifts.

The parser set is not enumerable. Which parsers exist, and which knobs each honors
(sheet_name defaults vary per lane — "years", "table", "indicator",
"api_response" — plus header_row, delimiter, sheets, selected_rows,
archive_member), is only answerable by reading both chains end to end.

Proposal

A single registry, one entry per lane:

PARSER_LANES: dict[str, ParserLane] = {
    "xlsx_table_full_rows": ParserLane(rows=..., cells_from_rows=True, ...),
    "xlsx_used_range": ParserLane(rows=None, cells=..., ...),
    ...
}

so that:

  • both methods dispatch through one table and can never disagree on the lane set;
  • "no rows for this lane" becomes an explicit declaration (rows=None) instead of a
    fallthrough that also swallows registration mistakes;
  • the shared rebuild-rows-then-select stanza exists once;
  • the lane set is enumerable — validation can reject unknown parsers by lookup, and a
    test can assert every registered lane is whole in both paths.

Pure refactor: no package YAML changes, no fact changes, bundle constants untouched.
Acceptance is the existing suite green plus the lane-wholeness test.

Severity and timing

Nothing is broken today — this is maintenance debt plus one latent silent-empty path,
not a live bug. Raised in review on #141 and again on #163 (deferred there as follow-up
scope). Worth doing before or at the start of wave 4, before the chain grows again.

Contributor guide

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Start in chronicle/source_package.py by reading build_source_rows and build_source_cells and tracing their parser branches and shared row-rendering logic. Define the registry and lane-wholeness validation described in the issue, then run the existing suite and the lane-wholeness test; done means the suite is green and both paths agree on every registered lane.

Written by the indexing model from the issue text.

Assessment

Tech stack
python
Domain
data
Issue type
Refactor
Difficulty
4/5
Estimated time
3-5 days
Activity status
Quiet
Clarity
Clearly specified
Newbie friendliness
54/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.