Repository navigation
fix(pdf): preserve a line-final hyphen that does not split a word (#4043) - #4052
Conversation
…cling-project#4043) Signed-off-by: Varanasi Sathveek <sathveekvaranasi@gmail.com>
|
✅ DCO Check Passed Thanks @VSathveek, all your commits are properly signed off. 🎉 |
Merge Protections🟢 All 2 merge protections satisfied — ready to merge. Show 2 satisfied protections🟢 Enforce conventional commitMake sure that we follow https://www.conventionalcommits.org/en/v1.0.0/
🟢 Require two reviewer for test updatesWhen test data is updated, we require two reviewers
|
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
ceberam
left a comment
There was a problem hiding this comment.
Thanks @VSathveek for suggesting this fix.
I added a minor non-blocking comment.
Signed-off-by: Varanasi Sathveek <sathveekvaranasi@gmail.com>
|
It's perfect |
|
Confirmed on 2.123.1 — the silent data loss from the original report is resolved, thank you. Residual behavior matches exactly what you flagged in the PR description: all three of my original repro cases now come out as - flagname (detached, space preserved) rather than -flagname (closed up). Not blocking for me — this is now deterministically catchable/correctable downstream, which is a big improvement over silent corruption. But since you called out the pre-strip whitespace preservation as a larger change you didn't want to fold into this PR, I'd be happy to take a pass at it if that's still open. I don't have full context yet on where the .strip() call happens in the caller before sanitize_text sees each cell — if you can point me at the right spot, I can look into preserving that whitespace evidence so the join logic can tell "close up" (-prio) from "keep the space" (- ECCV) apart properly, rather than defaulting to one behavior for both. Happy to open a draft PR to get early feedback once I've got a shape for it, rather than surprising you with something fully baked. |
|
Thanks for checking it against 2.123.1, and good to hear the residual shape is what you expected rather than a surprise. The follow-up is still open — please go ahead, and a draft PR to agree the shape first sounds like exactly the right way to do it.
textlines = [
cell.text.replace("\x02", "-").strip() # L201 - the trailing space dies here
for cell in cluster.cells
if len(cell.text.strip()) > 0
]
text = self.sanitize_text(textlines)Three things that cost me time in that area and may save you some:
Reference data is the other thing to plan for: four documents moved in my PR, and anything that changes this join will move them again. Running the suite against unmodified Tag me on the draft and I will review; I know this corner reasonably well now. One thing so we do not collide: I am going to look at |
|
Follow-up is up: #4180 applies the same attached-hyphen check to |
Issue resolved by this Pull Request:
Resolves #4043
Problem
PageAssembleModel.sanitize_text()drops a line-final hyphen before joining twolines, assuming it is a hyphen that splits a word across the break. The guard
deciding this inspects the last word token anywhere in the line rather than
the hyphen itself:
\b[\w]+\bskips over whitespace, so for a line ending... -pn 1 -the last tokenis
"1", the guard passes, and the hyphen is deleted even though it was neverattached to a word. The wrap reported in the issue is therefore extracted as
... -pn 1 prio 3 ..., the flag silently losing its dash.The deletion is not specific to CLI syntax. Any line-final hyphen that follows
whitespace is affected, and the existing test corpus turns out to contain four
such documents where a real character is being discarded today.
Fix
Only a hyphen attached to the preceding word marks a split word, so the fix
requires the character immediately before it to be alphanumeric:
That condition only ever narrows the existing one, so nothing de-hyphenated
today stops being de-hyphenated:
"algo-"+"rithms"→"algorithms"isunchanged, as is the existing exclusion of underscore-bearing tokens
(
"foo_bar-"+"baz"stays"foo_bar-baz").A detached hyphen is then treated as what it is — an ordinary character that
happens to sit at a line break — so it is kept and the lines are joined with a
space, exactly as the non-hyphen branch already does.
Reference data
Four documents change. Three recover a character that is being discarded today:
redp5110_sampledimproves structurally. Four bullet items whose-marker satalone on its own cell were glued to the following word, which pushed them out of
the list and into numbered entries; they are now recognised as list items:
One line moves sideways rather than forwards, and I would rather point at it than
let it pass unnoticed. In
2305.03393v1the author ORCIDs arrive as separatecells (
"[0000","-","0002", ...), so the bare hyphens now take a space:Neither form is the real
[0000-0002-3723-6960]; the identifier is alreadybroken upstream by the per-cell split and the space this branch adds between
cells. No character is gained or lost, so I left it rather than special-casing
digits, but it is the one diff here that is not an improvement.
I ran the suite against unmodified
mainfirst to confirm this environmentreproduces the committed reference data exactly, so these diffs are attributable
to the change and not to local drift. Flagging them for the double review the
contributing guide asks for.
A limitation worth stating
Whether a detached hyphen should close up against the next token (
-prio) orkeep its space (
- ECCV) is not decidable at this layer: the caller.strip()seach cell before
sanitize_textsees it, so the trailing-space evidence isalready gone. All four corpus cases are separator dashes wanting the space, so
that is the default here, and the issue's flag case comes out as
-pn 1 - prio 3— the dash preserved but not closed up. That fixes the data loss the issue is
about without inventing a distinction the input no longer carries. Recovering it
properly means keeping the pre-strip whitespace, which is a larger change to the
caller and I would rather not fold it in here.
Tests
TestSanitizeTextHyphenationintests/test_page_assemble_model.pycovers bothdirections: an attached hyphen still joins a split word, a non-alphanumeric
token is still left alone, and detached hyphens (wrapped flag, separator dash,
bullet marker, negative number) are preserved. The detached cases fail on
main.One related spot, left alone deliberately
ReadingOrderModel._merge_elementshas a parallel end-of-line hyphen check formerging elements across cluster boundaries and drops a detached hyphen the same
way. I have not touched it here, since the module has other work in flight
(#3891, #3955). Happy to follow up separately, or to fold it in if you prefer.
Checklist: