fix(sheet): preserve merged-cell spans that extend past the used range - #16
Conversation
|
Closing in favor of #13, which was opened first and takes the same two-layer approach (sheet-side extent + the finish() trailing-trim) — I hadn't seen it when I opened this. One deliberate difference worth carrying over from this PR if useful: an integration fixture (handmade-overhang.xlsx) plus a to_document() retention test asserting the 3×10 grid and spans — happy to contribute those to #13 as a follow-up if wanted. Verified #13 against the issue's openpyxl repro locally; it resolves it. |
|
Reopening: #13, which this was closed in favor of, was closed when its author's account was deleted, so #8 is unaddressed again. Since then I've folded in the one behavior where #13 had the better call — an oversized region (a crafted whole-sheet merge, or a real whole-column merge) now degrades to today's clipped form instead of failing the conversion with ResourceLimit (c19fbb0). Branch is rebased on current main and the full suite passes. |
A merge anchored on populated cells but reaching into empty rows or columns was intersected with the used range, so a valid source merge (F1:O3 with only F1 populated) silently collapsed to an ordinary 1x1 cell. Two layers clipped it: - the sheet frontend clamped the region's end to the used range; - GridBuilder::finish dropped trailing rows holding only covered positions and re-clamped the surviving spans. Keep the region's full extent (the clamped top-left still decides the origin, and regions that do not intersect the used range stay ignored), give the overhanging rows a place to materialize, and exempt span- claimed rows from the trailing trim - covered positions are structure a producer declared. Runaway regions stay bounded: place() charges the whole span against MAX_EXPANSION before any expansion, so a crafted whole-sheet merge fails as a resource limit. The rendered Markdown is unchanged (the renderer trims the empty tail visually); the fix is observable through to_document(). Fixes firecrawl#8
A region whose full extent would blow the expansion budget (a crafted whole-sheet merge, or a real whole-column merge) previously failed the conversion with ResourceLimit. Files that converted before this branch must keep converting: such a region now degrades to the clipped form - the populated rectangle - and only loses the overhang.
c19fbb0 to
067b07d
Compare
Fixes #8.
What
A merge anchored on populated cells but reaching into empty rows or columns
was clipped to the used range, so a valid source merge (
F1:O3with onlyF1populated) silently collapsed to an ordinary 1×1 cell into_document().The collapse happened in two layers, so both change:
src/formats/sheet/mod.rs) intersected each mergedregion with calamine's used range before building the grid — the clip the
issue reports.
GridBuilder::finish(src/model/table.rs) dropped trailing rowsholding only covered positions ("all-empty" treated covered slots as
empty) and then re-clamped the surviving spans — so even an unclipped
span would have shrunk back.
The fix
defenses are preserved unchanged: the clamped top-left still decides the
origin, and regions that don't intersect the used range at all are still
ignored (the
merge_outside_the_used_columns_is_ignoredtest isuntouched). Rows below the used range are appended so the overhang has
positions to materialize into.
finish()'s trailing trim now exempts span-claimed rows: coveredpositions are structure a producer declared, and dropping them silently
shrinks a valid span. Rows of plain empty cells still trim (new test pins
that).
Runaway regions stay bounded — a region whose full extent would blow the
expansion budget (a crafted
A1:XFD1048576merge, or a real whole-columnmerge) degrades to today's clipped form instead of materializing: the file
still converts, it just loses the overhang (new test proves it).
Behavior
The reporter's exact openpyxl repro, before → after:
The rendered Markdown is unchanged — the renderer already trims the
trailing empty area visually, so
to_markdownstill emits| Merged heading |. The fix is observable throughto_document(), whichis the surface the issue is about. Consequently no existing snapshot
changed.
Tests
1×1-anchor shape (3×10 span, 29 covered), the existing out-of-range-merge
defense unchanged, and the whole-sheet crafted merge degrading to the
clipped form instead of erroring
(
oversized_merge_degrades_to_the_clipped_form).finish()(replaces the testthat asserted the old clamping behavior); trailing empty-origin rows still
trim.
handmade-overhang.xlsxfixture + snapshot, and amodel-retention test asserting the 3×10 grid and spans via
to_document().cargo fmt --check,clippy --workspace --all-targets --all-features -D warnings, andcargo test --lockedall pass; only the new snapshotwas added, nothing else moved.
Note
This branch is rebased on current main and composes with the new
resolve_header_rowsdetection (fe97550) — header resolution runs on thefinished grid, including the preserved overhang rows.
Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.