Skip to content

fix(format): support thousands grouping and format sections in numberFormat/TEXT (HF-287)#1716

Open
marcin-kordas-hoc wants to merge 1 commit into
developfrom
feat/hf-287-numberformat-sections
Open

fix(format): support thousands grouping and format sections in numberFormat/TEXT (HF-287)#1716
marcin-kordas-hoc wants to merge 1 commit into
developfrom
feat/hf-287-numberformat-sections

Conversation

@marcin-kordas-hoc

@marcin-kordas-hoc marcin-kordas-hoc commented Jul 24, 2026

Copy link
Copy Markdown
Collaborator

What & why

TEXT / numberFormat only understood a single simple mask ([#0]+(\.[#0]*)?); complex masks emitted garbage — e.g. TEXT(1234.5,"#,##0.00")"1235,##0.00". It ignored thousands grouping (#,##0), format sections (;), and HF's configured decimalSeparator/thousandSeparator.

How (Option A — extend the formatter in place; no grammar rewrite, no public API, no i18n)

  • src/format/parser.ts: widen numberFormatRegex to admit , into the flat character class ([#0,]+(\.[#0]*)?) — no nested quantifier (keeps the DEV-2120 ReDoS discipline); white-box shape test via exported NUMBER_FORMAT_REGEX_SOURCE.
  • src/format/format.ts: section split (positive;negative;zero, quote/escape-aware), grouping with config.thousandSeparator, decimal via config.decimalSeparator, sign on abs, color-tag strip applied AFTER the stringifyCurrency callback (doesn't disturb that extension point), parse-failure falls back to the cleaned format string.
  • Docs (compatibility-with-microsoft-excel.md) reworded as nuances; CHANGELOG under Fixed.

Verified against Excel (customer repro table)

format value Excel now
#,##0.00 1234.5 1,234.50 ✅ (thousand ,)
#,##0.00;-#,##0.00 -1234.5 -1,234.50
#,##0.00 "zł" 1234.5 1 234.50 zł ✅ (thousand )
$#,##0.00;-$#,##0.00 -1234.5 -$1,234.50
000.00 -5 -005.00

New spec: 36 tests. Format/TEXT regression: 203 green, 0 regressions.

⚠️ Paired hyperformula-tests change required

One existing assertion flips (correct-behavior change): function-text.spec.ts "works for number format" — TEXT(12.45,"$###,##0.00") was '$12,##0.00' (garbage), now '$12.45'. That test lives in the private hyperformula-tests repo and needs the paired update, or unit-tests CI will fail on the old expectation.

Scoped OUT (per ADR — not regressions)

percent 0.00%, scaler arithmetic (degrades visibly), ? placeholders, scientific E+, @/4th text section, [condition] comparators, and placeholder chars inside quoted literals. Also: =TEXT(x,"… ""zł""") as a literal formula still returns #ERROR! — HF's formula parser rejects embedded "" (unrelated to the formatter, which does handle the quoted literal when the format arrives via config/programmatically).


Note

Medium Risk
Changes core format() dispatch and number rendering used by TEXT, with intentional output changes for previously broken masks; scope is localized to formatting with broad test coverage.

Overview
Fixes TEXT / built-in number formatting so complex Excel-style masks no longer leak unparsed fragments (e.g. #,##0.001235,##0.00).

The number path now splits format strings on ; (quote/escape-aware) and picks positive/negative/zero sections, groups thousands when #,##0 is used and thousandSeparator is set, uses decimalSeparator / thousandSeparator from config, formats negatives on magnitude with correct sign handling, and strips [Red]-style color tags after stringifyCurrency but before date/time dispatch. Placeholder-less sections render as literals; the parser regex admits grouping commas with a flat pattern and skips non-placeholder runs.

Docs and CHANGELOG document Excel parity nuances and limits (percent, scaler commas, etc.). New test/hf-287-numberformat.spec.ts covers oracles, TEXT integration, and ReDoS regex shape.

Reviewed by Cursor Bugbot for commit fa890f6. Bugbot is set up for automated code reviews on this repo. Configure here.

@netlify

netlify Bot commented Jul 24, 2026

Copy link
Copy Markdown

Deploy Preview for hyperformula-dev-docs ready!

Name Link
🔨 Latest commit fa890f6
🔍 Latest deploy log https://app.netlify.com/projects/hyperformula-dev-docs/deploys/6a63aac330840f000844371a
😎 Deploy Preview https://deploy-preview-1716--hyperformula-dev-docs.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify project configuration.

@qunabu

qunabu commented Jul 24, 2026

Copy link
Copy Markdown
Contributor

@marcin-kordas-hoc

Copy link
Copy Markdown
Collaborator Author

bugbot run

@marcin-kordas-hoc
marcin-kordas-hoc force-pushed the feat/hf-287-numberformat-sections branch from 98bb963 to 8a64987 Compare July 24, 2026 12:32
Comment thread src/format/parser.ts
Comment thread src/format/format.ts Outdated
@github-actions

github-actions Bot commented Jul 24, 2026

Copy link
Copy Markdown

Performance comparison of head (fa890f6) vs base (23a7437)

                                     testName |    base |    head | change
--------------------------------------------------------------------------
                                      Sheet A |  496.97 |   495.9 | -0.22%
                                      Sheet B |  158.71 |  159.85 | +0.72%
                                      Sheet T |  142.24 |  142.84 | +0.42%
                                Column ranges |  473.32 |  472.04 | -0.27%
                                Sorted lookup | 14336.4 | 14277.2 | -0.41%
Sheet A:  change value, add/remove row/column |   15.88 |   15.89 | +0.06%
 Sheet B: change value, add/remove row/column |  136.89 |  137.36 | +0.34%
                   Column ranges - add column |  149.41 |  159.34 | +6.65%
                Column ranges - without batch |  460.41 |  474.66 | +3.10%
                        Column ranges - batch |  116.59 |  116.57 | -0.02%

@marcin-kordas-hoc
marcin-kordas-hoc force-pushed the feat/hf-287-numberformat-sections branch from 8a64987 to b92d6c0 Compare July 24, 2026 12:54
…Format/TEXT (HF-287)

The TEXT number formatter understood only a single simple mask
(`[#0]+(\.[#0]*)?`), so complex masks leaked their unparsed tail into the
output (e.g. `TEXT(1234.5,"#,##0.00")` -> `1235,##0.00`) and it ignored
the instance's configured separators.

Extend the existing formatter in place (Option A):
- parser.ts: widen the number-format regex to a FLAT class `[#0,]+(\.[#0]*)?`
  that admits the grouping comma (no nested quantifier — DEV-2120 ReDoS
  discipline). Export its source for a white-box shape test.
- format.ts: strip presentational color tags (`[Red]`, ...) after the currency
  callback and before date/time dispatch; split the mask into sign-selected
  sections (positive;negative;zero) honoring quotes/escapes; thread Config so
  the decimal glyph uses `decimalSeparator` and grouping uses `thousandSeparator`
  (empty on default config -> no visible glyph). Sign is extracted on `abs`,
  fixing the pre-existing `padLeft('-5',3)` bug (`TEXT(-5,"000.00")` -> `-005.00`).
  Trailing scaler commas degrade to a visible literal rather than silently
  mis-scaling. Parse failures fall back to the cleaned format string.

No public API change, no i18n, no grammar rewrite. Percent scaling, scaler
arithmetic, `?` placeholders, scientific notation and `[condition]` comparators
remain out of scope.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@marcin-kordas-hoc
marcin-kordas-hoc force-pushed the feat/hf-287-numberformat-sections branch from b92d6c0 to fa890f6 Compare July 24, 2026 18:11
@marcin-kordas-hoc

Copy link
Copy Markdown
Collaborator Author

bugbot run

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ Bugbot reviewed your changes and found no new issues!

Comment @cursor review or bugbot run to trigger another review on this PR

Reviewed by Cursor Bugbot for commit fa890f6. Configure here.

@marcin-kordas-hoc
marcin-kordas-hoc marked this pull request as ready for review July 25, 2026 10:54
@codecov

codecov Bot commented Jul 25, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 97.23%. Comparing base (23a7437) to head (fa890f6).

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #1716      +/-   ##
===========================================
+ Coverage    97.21%   97.23%   +0.01%     
===========================================
  Files          178      178              
  Lines        15611    15676      +65     
  Branches      3460     3440      -20     
===========================================
+ Hits         15177    15242      +65     
+ Misses         434      426       -8     
- Partials         0        8       +8     
Files with missing lines Coverage Δ
src/format/format.ts 99.54% <100.00%> (+0.19%) ⬆️
src/format/parser.ts 100.00% <100.00%> (ø)

... and 5 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants