refactor(report): name the report's classes after the canonical contract (#689) - #703
Draft
willhea wants to merge 6 commits into
Draft
refactor(report): name the report's classes after the canonical contract (#689)#703willhea wants to merge 6 commits into
willhea wants to merge 6 commits into
Conversation
A prototype for a decision, not a finished change. It applies the whole mapping so the effect and the cost are both visible; the tiers are separable and any of them can be dropped. Tier A renames abbreviations and one word that disagreed with the contract outright: diff-add/mod/del become diff-added/modified/removed, the fb- prefix becomes full-text-line and its parts, and the toc- prefix becomes tree-. The toc rename is the one worth arguing for on its own. <toc> is a real GPO element in the bill XML, carrying the bill's own table of contents, and the corpus fixtures contain it. The class named the navigation tree instead, so it was not an abbreviation of our own word but a collision with a source word that means something else. Renaming it returns toc to the thing GPO uses it for. Tier B replaces UI-pattern names with what the contract calls the thing: change-card becomes change, card-group becomes change-group, and the badge family becomes change-type, which is the contract's own field name. Tier C emits the level the nav already had and was discarding. The canonical contract records tree.level as shared GPO vocabulary, and the renderer received it and dropped it on the floor, so nothing downstream could tell a title from an account without reading the label text. Nav entries now carry level-title, level-account, level-section and the rest. Renders are pixel-identical for both the PDF and XML reports, verified against a control that recolours --background and does differ, so the comparison is not blind. Gates: fast 2009 passed, browser 41 passed with --run-browser, ruff and format clean. Collection is 3850 before and after, so no test was added or lost. Four couplings had to be found by running things rather than by searching, and they are the argument for keeping the riskiest rename optional: - badge-<type> is built by concatenation, so the CSS renamed and the markup did not. The reports rendered without badge colours and the pixel check caught it. - test_frontend_browser.py imported the renamed builder function. The fast tier excludes browser tests, so that would have merged green. - Seven browser tests select .view-toggle__btn[data-view="full"]. - Two bare 'full' string comparisons in the sidebar JS decide which pane is hidden. Nothing that searches for a class name would find them. Refs #653 (the view re-deciding what the diff already settled), #676. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
#689) Settles the prototype into the agreed set: tiers A and B, minus the view-full rename, plus tier C. view-full to view-full-text bought the least clarity of any rename in the set and cost the most: the class, the data-view attribute its name is concatenated from, two bare 'full' comparisons in the sidebar JS that decide which pane is hidden, and the browser tests' attribute selectors. Two of those four are string literals that no search for a class name would surface, and the JS pair failed silently rather than loudly: the tree sidebar simply never unhid. Everything else stands. full-text elsewhere is a different rename, off full-bill, and stays: the view container, its lines, pages and meta. Renders stay pixel-identical for both the PDF and XML reports against a control that recolours --background and does differ. Gates: fast 2009 passed, browser 41 passed with --run-browser, ruff and format clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…asses (#689) Defensive review of the naming change, applied. The report already had a convention for this and the change had not followed it. data-type carries a change type verbatim on 74 elements and the Structural filter reads it, so a contract value belongs in an attribute under the contract's own field name, with the contract's own value. The nav level moves from a level-title class to data-level="title". The field name and the value are now the schema's, unmangled into one token, which is the point of emitting it at all: a reader or a model sees the pair the contract defines. It also leaves the class namespace alone, which matters because GPO's own bills.css uses bare .title and .division; [data-level="title"] can adopt those rules without colliding, where level-title could not have inherited them at all. That was the stated reason for wanting the level in the first place. The change type was being stated three times on one card: a bare class styling the left border, data-type driving the filter, and a modifier class on the badge. Both stylesheets now select on data-type and the value is stated once. unanchored stays a class, being a display state rather than a contract value. View switching no longer rebuilds a class name from an attribute value. Each pane declares the view it belongs to and the JS compares attributes, so the class, the attribute and two bare 'full' literals no longer have to move together. That coupling broke the sidebar silently during the prototype: the tree pane simply never unhid. A pane declares a view only when a second view exists to switch to, since otherwise the pairing asserts something untrue. Two tests were relaxed by the prototype to tolerate the appended level, which removed their ability to notice a wrong class. With the level in an attribute the class is fixed again, so they now pin it exactly while allowing attributes after it, which is stricter than either earlier form. Pixel-identity proves the rename but cannot see the level, which has no styling, so it was not evidence for it. Two tests now assert the level per entry rather than by count: a count cannot catch two entries swapping levels. Both were shown to fail against a renderer emitting a fixed level, then restored. Gates: fast 2011 passed, browser 41 passed with --run-browser, ruff and format clean, both reports still pixel-identical against a live control. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The #422 money tests split the changes view on the literal `class="change`, which was unambiguous while the card class was `change-card`. Renaming the card to `change` made that string a prefix of four sibling names, so the split cut at change-group, change-type, change__header and change__body as well. A 37-change report shattered into 260 chunks, each starting mid-attribute. A card's <h3> landed in one chunk and its dollar amounts in the next, and since the tests keep only chunks whose heading names the section they want, the amounts read as absent. The failure said "old-side amount $4,933,113,000 missing from the sec. 256 card", which is precisely the defect #422 exists to catch, reported for the wrong reason. Split on `id="change-<n>"` instead. Only a card carries it, so the anchor holds whatever the classes are called. 37 changes now yield 37 chunks and each amount sits under its own heading. The prefix overlap itself is not the defect and is not new: `--diff-add` is a prefix of `--diff-add-foreground`, and `view` of `view-toggle`, without ever causing trouble, because those consumers match whole tokens. This was the one place in the repository reading a class name as a substring. Caught by CI rather than locally because both tests are slow-marked and only four of the five gates had been run. All five pass on this tree: ruff, format, fast 2011, browser 41, slow 1750. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…me hollowed out Review feedback on #703, both findings accepted. P2. level_attr interpolated tree.level into an attribute unescaped, and its own docstring argued that was safe because the vocabulary is closed. That is a fact about today's writers, not a property of the function: format_diff_html takes a canonical document, and that contract is published and versioned precisely so documents can arrive from elsewhere. The label two lines above was already escaped, so this also brings the field in line with its neighbour. Confirmed by injecting a level of `x" autofocus onfocus=alert(1)`, which produced a live autofocus and onfocus attribute on the nav entry. Escaped, the whole value survives as one attribute. The absent-level test is replaced rather than joined by a second one, so the count does not grow: both halves guard the same seam, how a level reaches the attribute. P3. Three assertions read `assert "change" in html`. Each was `assert "change-card" in html` before the #689 rename, and each failed on a report containing no cards. Renaming the card class to `change` made them tautologies, because change-filter, change-group, change-type and change__body all appear in the embedded stylesheet: verified that "change" is present in a report rendered from an empty diff, where 'id="change-0"' is not. They now anchor on that id, which only a card carries. One assertion in the new escaping test was wrong in the same way the defects it guards are wrong, and the fast gate caught it: `" autofocus" not in html` treats a substring as structure, and once escaped that text is legitimately present as data. It now asserts the whole escaped attribute survives intact. All five gates on this tree: ruff, format, fast 2011, browser 41, slow 1750. The slow run was repeated after the last edit, because mutations made while an earlier background run was in flight left its result describing a tree that no longer existed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The container was `removed-changes` and each item `removed-change`: one letter
apart, the singular a strict prefix of the plural, and adjacent in the
stylesheet. The BEM suffixes made it worse rather than better, since
`removed-changes__note` belongs to the container and `removed-change__head` to
the item, distinguished by that same letter buried mid-name. Both names are
valid, so a slip between them produces no error.
Naming the item as an element states the containment the `__` separator exists
to express, and leaves one block name in the family:
.removed-changes .removed-changes
.removed-changes__note .removed-changes__note
.removed-change -> .removed-changes__item
.removed-change__head .removed-changes__item-head
This replaces a distinction the earlier names had. Before #689 they were
`removed-appendix` and `removed-block`, clearly different words; renaming both
toward contract vocabulary is what collapsed them onto near-twins.
The rename carries its own hazard, which is the point: a plain replace of
`removed-change` turns `removed-changes` into `removed-changes__items`. Matching
is token-anchored and the container count is asserted unchanged.
Pixel-identity does not cover this. The screenshots capture the first screenful
of the changes view and this section sits at the foot of the full-text view, so
it is verified instead by census: 31 items and 31 heads renamed, container
untouched, and the whole-document class census reconciles to zero difference
under the mapping. Every emitted class in the family has a rule and no rule is
orphaned.
All five gates: ruff, format, fast 2011, browser 41, slow 1750.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this does
Renames the report's CSS classes to the vocabulary the canonical diff contract already
uses, and carries the contract's structural level on each navigation entry.
Nothing changes visually. Both the PDF and XML reports render pixel-identical.
Closes #689. Refs #653, #676.
Why the contract decides the names
schema/canonical-diff.mdis the versioned JSON shape every DeltaTrack consumer reads,and it already fixes this vocabulary.
change_typeis the enumadded | removed | modified | moved. Thetreenode'slevelfield is documented in the schema itselfas "Shared GPO vocabulary":
division,title,major,agency,account,section,subsection,grouping,preamble,heading. Those are the words the source materialuses for the parts of an appropriations bill, so none of this is invented here.
Class renames
Abbreviations, and one word that disagreed with the contract outright
diff-add/diff-mod/diff-deldiff-added/diff-modified/diff-removedfb-row/fb-text/fb-gutter/fb-pagefull-text-line/full-text-line__text/full-text-line__number/full-text-pagefull-bill/full-bill-meta/full-bill--no-gutterfull-text/full-text-meta/full-text--no-line-numberstoc/toc-child/toc-group/toc__title/sidebar-toctree/tree-node/tree-group/tree__title/sidebar-treediff-delwas not only an abbreviation: the contract's word isremoved.The
tocrename is worth its own paragraph.<toc>is a real GPO element in billXML, carrying the bill's own table of contents, and the committed corpus fixtures contain
it. The class named the navigation tree instead, so this was not an abbreviation of our
own word but a collision with a source word that means something else. Renaming it
returns
tocto the thing GPO uses it for.UI-pattern names replaced by what the contract calls the thing
change-card/change-header/change-bodychange/change__header/change__bodycard-group/card-group__labelchange-group/change-group__labelremoved-appendix/removed-blockremoved-changes/removed-changes__itemDeliberately unchanged, because they are already right:
section-number(thecontract's
section_number),v1/v2(the contract'sversions.v1/v2),citation,disclosure(the correct accessibility pattern name for a<details>element),
nav-*(an HTML landmark), andview-full(see below).Contract values are carried as attributes, not classes
The report already had this convention and it is now followed throughout:
data-typecarries a change type verbatim on 74 elements, and the Structural filter reads
el.dataset.type. A contract value belongs in an attribute under the contract's ownfield name, with the contract's own value.
data-levelis new. The renderer already received each node'sleveland discardedit, so nothing downstream could tell a title from an account without parsing the label
text. Nav entries now carry it verbatim.
This shape, rather than a
level-titleclass, is also what makes GPO stylinginheritable:
bills.cssdefines bare.titleand.division, so[data-level="title"]can adopt those rules by a mechanical transform where a prefixed class could not have.
Checked: 205 GPO classes, zero collisions with the 93 the report emits.
change_typeis now stated once. It had been on a card three times: a bare classstyling the left border,
data-typedriving the filter, and a modifier class on thebadge. Both stylesheets select on
data-type.unanchoredstays a class, being adisplay state rather than a contract value.
View switching no longer rebuilds a class name from an attribute value. Each pane
declares its view and the JS compares attributes, so the class, the attribute and two
bare
'full'literals no longer have to move together. A pane declares a view only whena second view exists to switch to.
levelis escaped on the way in. Today's writers draw it from this repository's ownliterals, but
format_diff_htmltakes a canonical document, and that contract ispublished so documents can arrive from elsewhere.
Values emitted
data-levelsection148,account37,agency15,title5,major5,grouping4,subsection3,preamble1data-typemodified75,added67,removed11,moved7data-viewchanges3,full3What was dropped after prototyping
view-fullwas going to becomeview-full-textand is not. It bought the least clarityof any rename in the set and cost the most: four coupled sites, two of which are bare
string literals that no search for a class name would find.
Verification
A pure rename must change no pixel, so that is the check.
--backgroundPixel-identity proves the rename and is blind to
data-level, which has no styling, sothat is covered separately: one test asserts each entry's level per entry rather than
by count, because a count cannot catch two entries swapping levels; another asserts a
hostile value survives as one escaped attribute and an absent level emits none. Both were
shown to fail against a renderer emitting a fixed level and against one that skips
escaping.
All five gates on the final tree:
ruff checkclean,ruff format --checkclean,fast 2011 passed, browser 41 passed with
--run-browser, slow 1750 passed.Couplings that only running things found
Each would have shipped a silent defect, and none is findable by searching for a class
name.
badge-<type>was built by concatenation. The CSS renamed and the markup did not,so reports rendered with colourless change badges. The pixel comparison caught it.
test_frontend_browser.pyimported the renamed builder. The fast tier excludesbrowser tests, so that would have merged green.
[data-view="full"], and two bare'full'comparisons in the sidebar JS decide which pane is hidden. Those failed silently:
the tree sidebar simply never unhid. Together they are why
view-fullwas dropped.test_financial_diff.pysplit the changes view onclass="change. That string wasclass="change-cardbefore the rename and matched only cards; afterwards it alsomatched
change-group,change-type,change__headerandchange__body, shatteringa 37-change report into 260 chunks so a card's heading and its dollar amounts landed in
different ones. It now splits on
id="change-<n>", which only a card carries.assert "change-card" in htmlfailed on areport with no cards; after the rename
assert "change" in htmlpasses on one, becausechange-filterand friends appear in the stylesheet. They now anchor onid="change-0".The prefix overlap behind 4 and 5 is not itself the defect and is not new:
--diff-addisa prefix of
--diff-add-foreground, andviewofview-toggle, without ever causingtrouble, because those consumers match whole tokens.
Not in this PR
The report's ordering defect found while reviewing the prototype is filed separately as
#701 (changes the renderer cannot locate are listed after the whole bill). It is
pre-existing, unrelated to naming, and reproduces identically before and after this
change.
The HTML class vocabulary has no version, where the canonical JSON has
schema_version.Reports are saved and shared artifacts, so anything scraping
.change-cardout of a savedreport breaks silently on this change. Nothing in the repo does, and the JSON export is
the supported interface, but the class names are in practice a public surface with no
stability guarantee attached.