Conversation
…-j9wp) remove_location_fields in crates/pampa/tests/test.rs (used by test_qmd_roundtrip_consistency to compare JSON1 vs JSON3 modulo source info) was scrubbing only attrS, targetS, citationIdS — but the JSON writer emits seven more S-suffix keys for table internals: bodiesS, bodyS, captionS, cellsS, footS, headS, rowsS. captionS in particular is a scalar foreign-key into astContext.sourceInfoPool that sits directly on the Table object, so it survived the existing scrub as a dangling integer reference and could fail an otherwise content-stable round trip. This was deterministic, not flaky: same input always produced the same captionS:N on JSON1 and the same captionS:M on JSON3, because the two parses build differently-sized sourceInfoPools (the regenerated qmd has different source positions from the original) and the traversal-order IDs into each pool are stable. After scrubbing astContext (which removes the pools themselves), the bare IDs are references to nothing, but the test compared them numerically anyway. Add the missing seven keys to the scrub list, plus a regression fixture (table_with_inline_nbsp_in_cell.qmd) that exercises the gap — the fixture was originally written for #162 / bd-1aip but had to be dropped from that PR because of this scrub gap; it's reinstated here. Closes bd-j9wp.
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
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.
Summary
remove_location_fieldsincrates/pampa/tests/test.rs(used bytest_qmd_roundtrip_consistencyto compare JSON1 vs JSON3 modulo source info) was scrubbing onlyattrS,targetS,citationIdS— but the JSON writer emits seven more S-suffix keys for table internals:bodiesS,bodyS,captionS,cellsS,footS,headS,rowsS.captionSin particular is a scalar foreign-key intoastContext.sourceInfoPoolthat sits directly on theTableobject, so it survived the existing scrub as a dangling integer reference and could fail an otherwise content-stable round trip.This was discovered while implementing #165 (issue #162); the table fixture there had to be dropped because of this gap and is reinstated in this PR.
Not a determinism issue
Verified across 5 repeated runs that the same input always produces the same
captionS:Non JSON1 and the samecaptionS:Mon JSON3. The failure was deterministic but unscrubbed, not flaky:astContext.sourceInfoPoolhas 23 entries derived from the original qmd's source positions.astContext(which removes the pools themselves), the bare S-IDs are references to nothing, but the test compared them numerically anyway.Change
remove_location_fieldsto cover all ten S-suffix keys the JSON writer emits today (grep -oE '"[a-zA-Z]+S"' crates/pampa/src/writers/json.rsis the source of truth).remove_location_fieldscalling out the S-suffix convention so future writer-side additions extend this list too.crates/pampa/tests/roundtrip_tests/qmd-json-qmd/table_with_inline_nbsp_in_cell.qmdas a regression fixture. It failed at HEAD on thecaptionS:17 ≠ captionS:15mismatch and passes after the scrub-list extension.Verification
captionSmismatch, passes after the fix.cargo nextest run -p pampa: 3685 tests passing.cargo xtask verify --skip-hub-build --skip-hub-tests --skip-trace-viewer-build --skip-trace-viewer-tests: clean.Risk
Adding more scrubs is monotone-safe: a previously-passing fixture cannot start failing because the comparison only became less strict. The theoretical risk is that a future writer-side change names a content-bearing key with S-suffix, and this list silently swallows it. The doc-comment on
remove_location_fieldsflags the convention so a future reviewer extendingjson.rssees the dependency.Test plan
🤖 Generated with Claude Code