test: font-coverage corpus, and honour explicit spaces by default - #19
Merged
Merged
Conversation
The suite ran on 26 words in a single font, which is how two font-metric
bugs survived four releases. Adds a corpus that actually exercises the
font layer, and fixes the first bug it found.
Fixtures are hand-built rather than produced by a PDF library, because
the point is to exercise what a producer normally hides: the standard-14
pages carry no /Widths and no /FontDescriptor, which is spec-legal and is
exactly the case that hid both metric bugs. A reportlab fixture would
embed the metrics and quietly test nothing. Three sizes per page, since a
descender scaled by the wrong factor is indistinguishable from a wrong
constant at a single size.
They split by whether pdfplumber is a valid oracle. The 12 Latin fonts go
to testdata/golden and are diffed against it automatically -- 144 words
now match to 0.01pt on both axes. Symbol, ZapfDingbats and /Differences go
to testdata/fonts with no generated golden and direct Go assertions
instead, because pdfplumber decodes Symbol with StandardEncoding and
returns "abgdep" where the answer is Greek. Generating a golden there
would pin the wrong answer.
The corpus immediately found a real divergence, which is the point.
DefaultWordOpts did not set UseExplicitSpaces, so pdftable dropped space
glyphs and re-inferred boundaries from the gap alone. pdfplumber ends a
word AT a whitespace glyph before any gap test runs. At 8pt the space is
2.22pt wide, under the 3pt XTolerance, so a whole line collapsed into one
run. Body type is routinely 8-9pt, so this was not exotic. Turning it on
reproduces pdfplumber word-for-word and coordinate-for-coordinate.
That change then exposed two flaws in MergeSplitTokens, both found by
re-running against a real filing rather than the unit tests:
- boundarySplitsToken measured gaps without excluding whitespace
glyphs. A space is flush against its neighbours, so every space read
as a zero-gap intra-word join and real words were welded together.
Now a space sitting in the gap settles the question outright -- but
only in the gap, since a space elsewhere in either cell says nothing
about this boundary and blocking on it stops every merge.
- the join was unconditionally separator-less. Whether to merge is a
table-wide decision so the grid stays rectangular, but the SEPARATOR
has to be per-row: the same boundary can cut a token on one row and
fall between two real words on another. "(Dollars in millions," and
"except per share amount)" became "millions,except" because a split
several rows above had dropped the boundary.
3M 2018 10-K re-checked: 103/103 negatives intact, grid rectangular,
spacing correct.
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
📝 WalkthroughWalkthroughThe change adds generated font fixtures and extraction tests for standard PDF fonts. It enables explicit spaces by default and updates table-cell merging to preserve whitespace unless adjacent glyphs form one split token. ChangesFont extraction and spacing
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant PDFFixture
participant FontDecoder
participant TextExtractor
participant CellMerger
PDFFixture->>FontDecoder: provide encoded glyphs
FontDecoder->>TextExtractor: decode font glyphs
TextExtractor->>CellMerger: pass extracted glyphs and spaces
CellMerger-->>TextExtractor: preserve or merge boundary text
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Reviewer's GuideAdds a hand-built font-coverage PDF corpus to exercise real font metrics and encodings, tightens word-boundary handling to respect explicit space glyphs, and refines merge/split token logic so table reconstruction honours per-row separators and whitespace, all verified against pdfplumber and custom fixtures. Sequence diagram for updated mergeSplitTokens whitespace handlingsequenceDiagram
participant TableExtractor
participant mergeSplitTokens
participant boundarySplitsToken
TableExtractor->>mergeSplitTokens: mergeSplitTokens(rows, cells, chars, tol)
loop eachRow
loop eachCellBoundary
mergeSplitTokens->>boundarySplitsToken: boundarySplitsToken(chars, prevCell, cell, tol)
boundarySplitsToken-->>mergeSplitTokens: bool splits
alt [joining cells]
alt [text == "" or rowText[n] == ""]
mergeSplitTokens->>mergeSplitTokens: sep = ""
else [boundarySplitsToken == true]
mergeSplitTokens->>mergeSplitTokens: sep = ""
else [default]
mergeSplitTokens->>mergeSplitTokens: sep = " "
end
mergeSplitTokens->>mergeSplitTokens: rowText[n] += sep + text
end
end
end
mergeSplitTokens-->>TableExtractor: merged rows
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
CHANGELOG.md (1)
10-24: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winKeep one canonical
Unreleasedentry.The same release note is repeated under seven
## [Unreleased]sections. Keep the note in one current release section and remove the duplicates. Repetition can produce duplicate release output and makes the changelog history ambiguous.Also applies to: 104-118, 238-252, 397-411, 645-659, 1015-1029, 1427-1441
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@CHANGELOG.md` around lines 10 - 24, Keep the `DefaultWordOpts()` behavior-change note in a single canonical `## [Unreleased]` changelog section, and remove the duplicate copies from the other six `Unreleased` sections. Preserve the note’s content in the retained current release section while leaving unrelated historical entries unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@fonts_fixture_test.go`:
- Around line 165-169: Update the font-size validation around the sizes fixture
check to require the explicit presence of 8pt, 12pt, and 24pt, rather than only
checking that at least three sizes exist. Preserve the existing error-reporting
style and ensure the test fails if any required size is missing.
In `@page.go`:
- Around line 850-852: Update the whitespace classification in the
character-processing logic around c.Text so only characters with non-empty text
are checked with strings.TrimSpace and appended to spaces. Leave unmapped glyphs
with Text == "" out of spaces, preserving table-wide merge and separator
behavior.
---
Nitpick comments:
In `@CHANGELOG.md`:
- Around line 10-24: Keep the `DefaultWordOpts()` behavior-change note in a
single canonical `## [Unreleased]` changelog section, and remove the duplicate
copies from the other six `Unreleased` sections. Preserve the note’s content in
the retained current release section while leaving unrelated historical entries
unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 748e8b19-c6fa-40bc-9405-763674cbef93
⛔ Files ignored due to path filters (3)
testdata/fonts/differences.pdfis excluded by!**/*.pdftestdata/fonts/symbol.pdfis excluded by!**/*.pdftestdata/golden/fonts-standard14.pdfis excluded by!**/*.pdf
📒 Files selected for processing (8)
CHANGELOG.mdfonts_fixture_test.gomerge_split_test.gopage.goscripts/gen_font_fixtures.pytestdata/golden/fonts-standard14.expected.jsontestdata/golden/fonts-standard14.tables.expected.jsontext.go
Comment on lines
+165
to
+169
| // Multiple sizes matter: a descent scaled by the wrong factor is | ||
| // indistinguishable from a wrong constant at a single size. | ||
| if len(sizes) < 3 { | ||
| t.Errorf("fixture uses %d font sizes, want >= 3", len(sizes)) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Assert the required font sizes.
The cardinality check passes if a future fixture removes 8pt and adds another size. That removes the small-space regression coverage. Check for 8pt, 12pt, and 24pt explicitly.
Proposed fix
- if len(sizes) < 3 {
- t.Errorf("fixture uses %d font sizes, want >= 3", len(sizes))
+ for _, size := range []float64{8, 12, 24} {
+ if !sizes[size] {
+ t.Errorf("fixture does not render the required %gpt size", size)
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // Multiple sizes matter: a descent scaled by the wrong factor is | |
| // indistinguishable from a wrong constant at a single size. | |
| if len(sizes) < 3 { | |
| t.Errorf("fixture uses %d font sizes, want >= 3", len(sizes)) | |
| } | |
| // Multiple sizes matter: a descent scaled by the wrong factor is | |
| // indistinguishable from a wrong constant at a single size. | |
| for _, size := range []float64{8, 12, 24} { | |
| if !sizes[size] { | |
| t.Errorf("fixture does not render the required %gpt size", size) | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@fonts_fixture_test.go` around lines 165 - 169, Update the font-size
validation around the sizes fixture check to require the explicit presence of
8pt, 12pt, and 24pt, rather than only checking that at least three sizes exist.
Preserve the existing error-reporting style and ensure the test fails if any
required size is missing.
Comment on lines
+850
to
+852
| if strings.TrimSpace(c.Text) == "" { | ||
| spaces = append(spaces, c) | ||
| continue |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Do not classify unmapped glyphs as whitespace.
The Char contract defines Text == "" as a decoding failure, not as an explicit space. strings.TrimSpace("") == "", so Line 850 adds every unmapped glyph to spaces. This can suppress a valid table-wide merge or add an unwanted separator on a row that is already marked for merging.
Require non-empty text before the whitespace check.
Proposed fix
- if strings.TrimSpace(c.Text) == "" {
+ if c.Text != "" && strings.TrimSpace(c.Text) == "" {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if strings.TrimSpace(c.Text) == "" { | |
| spaces = append(spaces, c) | |
| continue | |
| if c.Text != "" && strings.TrimSpace(c.Text) == "" { | |
| spaces = append(spaces, c) | |
| continue |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@page.go` around lines 850 - 852, Update the whitespace classification in the
character-processing logic around c.Text so only characters with non-empty text
are checked with strings.TrimSpace and appended to spaces. Leave unmapped glyphs
with Text == "" out of spaces, preserving table-wide merge and separator
behavior.
This was referenced Aug 2, 2026
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.
The suite ran on 26 words in a single font. That is how two font-metric bugs survived four releases. This adds a corpus that actually exercises the font layer — and fixes the first bug it found.
The fixtures
Hand-built rather than produced by a PDF library, on purpose. The standard-14 pages carry no
/Widthsand no/FontDescriptor— spec-legal, and exactly the case that hid both metric bugs. A reportlab fixture would embed the metrics and quietly test nothing. Three font sizes per page (8/12/24), because a descender scaled by the wrong factor is indistinguishable from a wrong constant at a single size.They split by whether pdfplumber is a valid oracle:
testdata/golden//Differencestestdata/fonts/The second group gets no generated golden because pdfplumber is wrong there — it decodes Symbol with StandardEncoding and returns
abgdepwhere the answer isαβγδεπ, and123where the answer is dingbats. Generating a golden would pin the wrong answer.Coverage went from 26 words / 1 font to 170 words / 12 fonts / 3 sizes, and the 144 new words match pdfplumber to 0.01pt on both axes.
The bug it found
DefaultWordOpts()did not setUseExplicitSpaces. pdftable dropped space glyphs and re-inferred boundaries from the gap alone; pdfplumber ends a word at a whitespace glyph, before any gap test runs.At 8pt the space is
278/1000 × 8 = 2.22pt— under the 3ptXTolerance— so a whole line collapsed:pdfplumber's own gap test would not have split either (
90.22 > 87.99 + 3is false), which is what proves it split on the space glyph. Body type in real documents is routinely 8–9pt, so this was not an edge case. Turning it on reproduces pdfplumber word-for-word and coordinate-for-coordinate.Two follow-on flaws in MergeSplitTokens
Both found by re-running against a real filing, not by the unit tests:
boundarySplitsTokenmeasured gaps without excluding whitespace glyphs. A space is flush against its neighbours, so every space read as a zero-gap intra-word join and real words got welded together. Now a space in the gap settles it — but only in the gap: a space elsewhere in either cell says nothing about this boundary, and blocking on it stops every merge from happening at all (I made that mistake first).(Dollars in millions,+except per share amount)becamemillions,exceptbecause a split several rows above had dropped that boundary.Verification
go build,go vet,go test ./... -count=1 -race— green.TestStandard14FixtureCoversEveryLatinFontasserts the corpus breadth — 12 fonts, ≥3 sizes — so trimming the fixture cannot silently collapse coverage again.Note on scope
gen_golden.pyregenerates goldens for every PDF in the directory, including ones that had no committed golden before. I reverted those rather than silently rebaselining. One of them — word-level parity forissue-466-example— genuinely fails and is worth its own issue.Closes HAL-511
Summary by Sourcery
Add a dedicated font-coverage fixture corpus and align default word extraction behaviour with pdfplumber to prevent over-merging of small-type text.
New Features:
Bug Fixes:
Enhancements:
Summary by CodeRabbit