PolicyEngine / PolicyEngine/chronicle
Collapse source_package.py's parser dispatch chains into a lane registry
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
First steps
- Read the whole issue, then the project's contributing guide.
- Comment on the issue to say you are picking it up — it saves two people doing the same work.
- Fork the repository and make your change on a branch.
- 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