xlsx: normalise reversed dimension refs - #695
Conversation
jmcnamara
added
the
needs work for merge
|
The commit message is too verbose, here and in the other PRs that you submitted. Please write these yourself to describe concisely what has been changed or what is being fixed. You should be able to do that (for most changes) in 1-2 lines in the body. |
Reversed refs like C5:A1 underflowed the u32 extent arithmetic in get_dimension and Dimensions::len. The constructor now stores the corners ordered so start <= end on both axes.
e90bebe to
51c130c
Compare
|
Bad assumption on my part since ECMA-376 isn't clear. The closest authoritative statement is MS-OI29500 §2.1.1090, which says the standard doesn't clearly define what a valid cell range reference is, and that Office writes the smallest cell first. I've removed it. NBD since the normalization stands because it's a no-op on well-formed refs. The constructor now normalizes the corners. The struct fields are still public, so a hand-built Commit msg is now small. Working on the others. |
That would be an API change so it would need a wider discussion. However, in my opinion they should be private and replaced by accessors, and there shouldn't be any Also, once you have addressed the required changes can you mark them as resolved so it is clear that the work is done. |
krickert
changed the title
|
Review comments addressed, let me know what you think. |
Brings pipestream-main from 0.36.0 + the original fix commits to upstream/master at 0.36.1+ plus the post-review heads of the three open upstream PRs: tafia#695 dimension normalisation (Dimensions::new) tafia#696 grid-limit errors from cell reference parsing tafia#697 Range::from_sparse returns Result<Range<T>, RangeError> The tafia#697 head changes a public signature; the tree is byte-identical to upstream/master with the three fix branches merged (verified by diff), and the calamine test suite passes on that tree (46+164+65 tests).
cargo update -p calamine: pipestream-main f17b012 -> 4fddaec, which is upstream 0.36.1+ with the post-review heads of tafia/calamine#695, #696 and #697. One behavior change reaches the server: a declared dimension past the xlsx grid is now a hard ColumnNumberOverflow instead of a warn-and-continue, so the dimension_wide fixture moves its declaration to the in-grid maximum (A1:XFD1) to keep exercising the allocation path it tests. Suite green: 38 streaming + 5 unit tests.
2 participants
Fixes #692.
A
<dimension>whose end cell precedes its start, likeC5:A1, underflowed the u32 subtractions inget_dimension: a debug build panics and a release build reportsDimensions::len()values around 1.8e19.Dimensions::newnow orders the corners sostart <= endon both axes, andget_dimensiongoes through it, so a reversedrefparses the same as its ordered form. TheMAX_ROWS/MAX_COLUMNSwarnings compute from the normalised end corner.Dimensions::len()also widens to u64 before the+ 1, so a full axis no longer overflows.A
Dimensionsbuilt as a struct literal still bypasses the ordering; making the fields private is left to the wider API discussion from the review.Tests:
test_reversed_dimension_is_normalised(reversed on both axes and on each axis alone) andtest_dimensions_new_normalises_order.Touches the same
get_dimensionarm as #696; whichever lands second gets rebased.