Repository navigation
Conversation
…ions (storytold#817) S1: the built-in format table lives only in gridcraft-xlsx (file formats do not depend on numfmt); numfmt's unused copy and its exports are removed, xlsx gets tests pinning its lookup rules. S3: drop the render_number arm that printed what the fallback prints; literal text is appended to the output instead of built as a String per call. O3: render_number keeps pieces, text-before and literal replacements in one buffer (Parts), fill_int writes digits left to right, Section stores the date flags (sub-second digits, calendar, AM/PM) computed at parse time, render_date writes into one buffer. O4: significant_digits formats into a stack buffer; format_general_fit re-rounds the 15 digits per width into reused strings. An equality test compares format_value, TEXT() and format_general_fit with digests recorded from the previous implementation. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
vangel76
marked this pull request as draft
October 12, 2026 20:45
Port the scientific-exponent (storytold#773) and negative-zero (storytold#740) changes onto the single-buffer renderer; re-record 99 golden code rows (negative values that round to zero lose the minus sign, zero in ##0.0E+0 / #.##E+00 shows 0 in integer placeholders, 1904 dates end at 2957003). Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
|
Important Draft PR not reviewedDraft PRs are not automatically reviewed by default.
To automatically review draft PRs, update your CodeRabbit configuration: reviews:
auto_review:
drafts: true
Comment |
Contributor
Author
|
Merged main (conflicts in ROADMAP.md and the fraction renderer in numfmt, where main now keeps fraction numerators as f64). Re-recorded 4 golden rows for huge fractions such as 1e20, which now print exact digits. Ran: fmt check, workspace check, numfmt tests, full xtask ci (all 8 steps). |
Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
This branch has not been deployed
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.
Fixes #817
Findings S1, S3, O3 and O4 of the
crates/numfmtreview. No behaviour change; one PR because the S3 dead arm and theliteral_textcalls sit in the lines O3 rewrites.What changed, finding by finding
S1, one table of built-in formats. The two tables were identical (checked by diffing the entries).
cargo xtask layersforbidsgridcraft-xlsx -> gridcraft-numfmt(FormatDep; only a dev-dependency is allowed), so the table stays incrates/xlsx/src/tables.rsand numfmt's copy (builtin.rs,builtin_format,builtin_id; used by nothing but numfmt's own tests) is removed. Nothing else incrates/,apps/,docs/,xtask/referenced them (git grepoutside xlsx). xlsx's lookup rules are untouched and now pinned bytables::tests::builtin_format_ids(36 ids, 23-36 and 50 absent,builtin_idexact-case, ids 5-8 not written as built-ins). In numfmt's tests the three uses of the table became literal codes, andbuiltins(which tested the removed case-insensitive lookup) becamebuiltin_codes_render, which keeps the three rendering assertions.S3, dead arm and
literal_text. TheTok::At | General | Date | Comma => {}arm inrender_numberprinted nothing, which the fallback (the token's piece, empty) does too, because no code ever gives these tokens a piece. So it is removed whether or not the tokens can occur; the comment says thatAtcan occur (0@) andComma/General/Datecannot afteranalyze. NewTok::push_literal(&mut String);literal_textcalls it.render_text,render_general,render_dateandrender_numberappend through it and allocate nothing per token.O3, allocations per number.
render_number:pieces,beforeandlits(aStringper token) are now onePartsvalue: one text buffer and oneVecof byte ranges (+ the "this literal becomes k spaces" marker for fractions).fill_intwrites digits left to right (digit, then the separator after it) instead of building the reversed string and reversing it;fill_fracand the exponent text write into the buffer too.render_daterescanned the tokens three times per call. The three results (sub_digits, calendar part present, AM/PM present) are fields ofSection, computed once inanalyze. Date parts are written straight into the output (noStringper part).O4, General.
significant_digitsformats into a 64-byte stack buffer (the 800-digit exact-tie check keeps a heap string, it is the rare path) and fills a caller array instead of returning aVec.format_general_fitcomputed the 15 digits once already; the width loop now re-rounds them withfixed_into(the oldfixed_from_digitsbody, with the digitVecreplaced by a 16-byte array plus a zero count) into three strings that live for the whole call, instead of a newVecand twoStrings per width.fixed_from_digitsis a wrapper, sorender.rscallers are unchanged. The starting decimals are clamped to the existing 400 cap: zeros past it were trimmed anyway, and a width ofusize::MAX / 2no longer asks for a gigantic zero string (output identical, see the width list below).No allocation count or speed claim is made; this is argued by structure only (fewer per-token/per-iteration allocations), nothing was measured.
Tests
crates/numfmt/tests/render_equality.rs+tests/golden/*.txt: 152 format codes (numbers, thousands, percent, scientific, fractions, dates/times,[h]:mm, text sections, conditions, colours, fill*x, invalid codes) x 75 values (0, -0, negatives, tiny, huge, 0.5, 1.005, 2.675, 999.9995, NaN, +-inf, serials 0/59/60/61/2958465/2958466, ...) throughformat_valueandtext_functionin the 1900 system (and 1904 for date codes), plus text/bool/empty values; and 555 values (480 generated across magnitudes) throughformat_general_fitat widths 1..=30, 31, 40, 100, 399, 400, 401, 500, 1000. The golden files are FNV-1a digests of each row's full output, recorded from the code before this change (upstream/main9f896c1), not strings; one row per code / per value, so a failure names the row and prints its new output.NUMFMT_DUMP_DIRwrites the full text,NUMFMT_BLESS=1re-records. I checked the test fails when the thousands separator logic is changed on purpose. One extra code (a 420-placeholder format) was dropped from the grid by hand (and its golden row deleted) because Numfmt: no panic on a fraction denominator with 65,536 zeros; format codes are capped at 255 characters (#807) #809 caps code length;decimal::tests::roundinghas asserts for the beyond-400-decimals and 1e300 cases instead.decimal_digits/fixed/format_general_fitover ~9,000 values x 15 decimals x 44 widths was run locally with the olddecimal.rskept as a test module; it passed and was deleted (not committed).Commands run
cargo test -p gridcraft-numfmt -p gridcraft-xlsx -p gridcraft-calc -p gridcraft-chartcargo test -p gridcraft-engine format(9 tests; thenumfmtfilter matched none)cargo clippy -p gridcraft-numfmt -p gridcraft-xlsx --all-targets -- -D warningscargo fmt -p gridcraft-numfmt -p gridcraft-xlsx -- --checkcargo xtask layers,cargo xtask assetscargo check -p gridcraft-numfmt --target wasm32-unknown-unknownNot run: the full
cargo xtask ci, the full engine/ui-egui test suites,cargo xtask wasm.Interaction with other open PRs (trial merges with
git merge-tree)&switches to scientific past 21 characters #449, Data: “Remove trailing zeros after the decimal separator” conversion option (#656) #657, Data: Automatic Data Conversion options for typing, pasting and opening CSV (#506) #583.render.rs: Numfmt: scientific formats with a literal after E±, a % or a comma match Excel #773 and Numfmt: a negative number that rounds to zero shows no minus sign #740 (both rewrite thepieces/beforecode inrender_number; they would also stop compiling againstParts:pieces.get_mut(...),before.get_mut(first),let mut zero), and M12: regional formats (typed and displayed numbers, dates and times) #447 (render.rs,parse.rs). Whichever lands second needs a rebase onto the other.render_equalityfail once this is on main; re-record withNUMFMT_BLESS=1 cargo test -p gridcraft-numfmt --test render_equalityand review the changed rows.Not done / unverified
digit_indices(three smallVecs), theStringpair returned byfixed, and the denominator pieces in fractions (small strings), plus the result string.Atclaim (0@prints nothing for the token) is proven by structure and by the unchanged outputs of the rows with@in the grid; no number-section-with-@code is in the grid.🤖 Generated with Claude Code