HF-24: add stringifyCurrency config callback for TEXT#1665
Open
marcin-kordas-hoc wants to merge 40 commits into
Open
HF-24: add stringifyCurrency config callback for TEXT#1665marcin-kordas-hoc wants to merge 40 commits into
marcin-kordas-hoc wants to merge 40 commits into
Conversation
Contributor
✅ Deploy Preview for hyperformula-docs ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
3 tasks
Performance comparison of head (69d5916) vs base (00e5c73) |
marcin-kordas-hoc
force-pushed
the
feature/hf-24-stringify-currency
branch
from
0246ce0 to
09babfd
Compare
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 80fd34e. Configure here.
Contributor
|
Task linked: HF-85 Implement function DCOUNT |
sequba
requested changes
May 19, 2026
| * The INDEX function doesn't support returning whole rows or columns of the source range – it always returns the contents of a single cell. | ||
| * The FILTER function accepts either single rows of equal width or single columns of equal height. In other words, all arrays passed to the FILTER function must have equal dimensions, and at least one of those dimensions must be 1. | ||
| * Array-producing functions (e.g., SEQUENCE, FILTER) require their output dimensions to be determinable at parse time. Passing cell references or formulas as dimension arguments (e.g., `=SEQUENCE(A1)`) results in a `#VALUE!` error, because the output size cannot be resolved before evaluation. | ||
| * The TEXT function does not accept embedded double-quote literals in the format string (e.g., `=TEXT(A1, "#,##0.00 ""zł""")` fails at parse time). Use the LCID-tagged form (`[$zł-415] #,##0.00`) or supply a custom [`stringifyCurrency`](configuration-options.md#stringifycurrency) callback that handles such formats outside the parser. |
Contributor
There was a problem hiding this comment.
What is the LCID-tag? If I want to display polish currencies in a format "1234,56 zł", can I do it without stringifyCurrency?
Collaborator
Author
There was a problem hiding this comment.
Good catch — clarified inline in known-limitations.md: defined the LCID tag as a Microsoft Locale ID with a link to the MS-LCID spec, and explicitly noted that the Polish "1234,56 zł" pattern (decimal-comma) requires the stringifyCurrency callback because the built-in number formatter always emits . as the decimal separator. Fixed in f1eb4ef.
| | Coercion of explicit arguments | VARP(2, 3, 4, TRUE(), FALSE(), "1",) | 1.9592, based on the behavior of Microsoft Excel. | GoogleSheets implementation is not consistent with the standard (see also `VAR.S`, `STDEV.P`, and `STDEV.S` function.) | 1.9592 | | ||
| | Ranges created with `:` | A1:A2<br><br>A$1:$A$2<br><br>A:C<br><br>1:2<br><br>Sheet1!A1:A2 | Allowed ranges consist of two addresses (A1:B5), columns (A:C) or rows (3:5).<br>They cannot be mixed or contain named expressions. | Everything allowed. | Same as Google Sheets. | | ||
| | Formatting inside the TEXT function | TEXT(A1,"dd-mm-yy")<br><br>TEXT(A1,"###.###”) | Not all formatting options are supported,<br>e.g., only some date formatting options: (`hh`, `mm`, `ss`, `am`, `pm`, `a`, `p`, `dd`, `yy`, and `yyyy`).<br><br>No currency formatting inside the TEXT function. | A wide variety of options for string formatting is supported. | Same as Google Sheets. | | ||
| | Formatting inside the TEXT function | TEXT(A1,"dd-mm-yy")<br><br>TEXT(A1,"###.###”) | Not all formatting options are supported,<br>e.g., only some date formatting options: (`hh`, `mm`, `ss`, `am`, `pm`, `a`, `p`, `dd`, `yy`, and `yyyy`).<br><br>Currency formatting is opt-in via the [`stringifyCurrency`](date-and-time-handling.md#currency-integration) callback; without it, currency format strings fall through to the built-in number formatter.<br><br>Embedded double-quote literals (e.g. `#,##0.00 "zł"`) are not accepted by the parser; use the LCID-tagged form (`[$zł-415] #,##0.00`) instead. | A wide variety of options for string formatting is supported. | Same as Google Sheets. | |
Contributor
There was a problem hiding this comment.
Since we added stringifyCurrency, I think we can remove this line from List of Runtime Differences. Instead in the Compatibility with Ms Excel and Compatibility with Google Sheets guides, we should mention that the user need to provide both stringifyDateTime and stringifyCurrency to support all formats in the TEXT function.
Collaborator
Author
There was a problem hiding this comment.
Done. The TEXT-formatting row in list-of-differences.md is now a one-liner that points at both stringifyDateTime and stringifyCurrency. Added a new "TEXT function formats" subsection to both compatibility-with-microsoft-excel.md and compatibility-with-google-sheets.md noting that both callbacks together cover the full TEXT format range. Fixed in f1eb4ef.
Contributor
There was a problem hiding this comment.
I'd rather write something like "To support all date, time and currency formats, set stringifyDateTime and stringifyCurrency configuration options."
Collaborator
Author
There was a problem hiding this comment.
Done — updated to your suggested wording: "To support all date, time and currency formats, set stringifyDateTime and stringifyCurrency configuration options." Implemented in 8cbb139d4.
Comment on lines
+101
to
+103
| By default, the `TEXT` function renders only the simplest currency-looking formats — `"$0.00"`, `"$0"`, or `"$#.00"` (no thousands separator). Common Excel patterns such as `"$#,##0.00"` (with comma grouping), `"[$€-2] #,##0.00"` (EUR with German grouping), `"[$zł-415] #,##0.00"` (PLN), or accounting two-section formats like `"$#,##0.00;($#,##0.00)"` are **not** rendered correctly by the built-in number formatter; provide a [`stringifyCurrency`](../api/interfaces/configparams.md#stringifycurrency) callback to handle them. | ||
|
|
||
| HyperFormula itself ships with **no currency data** and **no currency library dependency**. You choose how to format: native `Intl.NumberFormat`, a third-party library, or a hand-rolled lookup table. |
Contributor
There was a problem hiding this comment.
IMHO these paragraph sound to negative. They say a lot about things that HyperFormula does not support. I'd rather say it in more positive tone like Out of the box HF supports all currency symbols through the currencySymbolconfig option and simple currency formats e.g.: "$0.00", "$0", or "$#.00". If your app needs more formats, you can define them using thestringifyCurrency configuration option. (do not use my exact wording; it's just an example of the more positive-sounding tone).
Collaborator
Author
There was a problem hiding this comment.
Reframed. The new currency-handling.md opens by stating what HF handles out of the box (simple $-prefixed formats — "$0.00", "$0", "$#.00") and frames stringifyCurrency as the additive extension point for locale-aware grouping, non-$ symbols, decimal-comma patterns, and accounting two-section formats. The reference table is preserved further down for users who want the side-by-side comparison, but no longer leads the page. Fixed in f1eb4ef.
|
|
||
| HyperFormula itself ships with **no currency data** and **no currency library dependency**. You choose how to format: native `Intl.NumberFormat`, a third-party library, or a hand-rolled lookup table. | ||
|
|
||
| The callback contract: |
Contributor
There was a problem hiding this comment.
Before discussing the callback, give a very simple example of the behavior with just currencySymbol provided and no custom stringifyCurrency.
Collaborator
Author
There was a problem hiding this comment.
Done. The new currency-handling.md now opens with a "Default behavior" subsection that demonstrates simple $-prefixed formats ("$0.00", "$#.00") producing "$1234.50" out of the box, before introducing the callback contract. The stringifyCurrency discussion follows for users who need richer formats. Fixed in f1eb4ef.
marcin-kordas-hoc
added a commit
that referenced
this pull request
May 20, 2026
Addresses review feedback on PR #1665: - Extract Currency integration section from date-and-time-handling.md into a new top-level guide currency-handling.md so the topic stands on its own and is reachable from the sidebar. - Open the guide with a positive framing: out-of-the-box support for simple $-prefixed formats, with stringifyCurrency as the additive extension point for richer patterns. Lead with a default-behavior example before introducing the callback. - known-limitations: explain what an LCID tag is inline and clarify that the Polish '1234,56 zł' (decimal-comma) pattern requires the stringifyCurrency callback because the built-in number formatter always emits '.' as the decimal separator. - list-of-differences: drop the TEXT-formatting paragraphs that were duplicating the new guide; replace with a one-liner pointing at stringifyDateTime + stringifyCurrency for full TEXT coverage. - compatibility-with-microsoft-excel and compatibility-with-google-sheets: add a 'TEXT function formats' subsection noting that both callbacks together cover the full TEXT format range. - Update ConfigParams.ts JSDoc cross-reference to point at the new currency-handling guide. Regenerated docs/api artifacts (gitignored) follow automatically via typedoc on docs:build.
✅ Deploy Preview for hyperformula-dev-docs ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
marcin-kordas-hoc
added a commit
that referenced
this pull request
May 25, 2026
Addresses the must-fix + should-fix findings from the parallel code-review-quality (A) and sherlock-review (C) passes on PR #1665. **P0 — `tryAccountingFormat` sign-loss bug** (A's only must-fix bug): docs adapter `customStringifyCurrency` in `currency-handling.md` mis-rendered negative values when the negative section was plain `$#,##0.00` (no parens). `Math.abs(value)` was formatted without ever adding the `-` prefix, so `value = -1234.5` against format `"$#,##0.00;$#,##0.00"` returned `$1,234.50` (positive-looking) instead of `-$1,234.50`. New branch: `parenMatch ? "(" + formatted + ")" : "-" + formatted`. Regression test for this path landed in hyperformula-tests `85979a0`. **`extract-doc-snippets.js` hardening** (C's main concerns): - Detect malformed `<!--snippet:NAME-->` markers (no spaces around the body) and fail loudly instead of silently skipping. Pre-fix, a typo in a docs author's marker meant the snippet was silently dropped from `test-utils/snippets/` while `snippets:check` still passed — drift undetected. - Skip symbolic links during `docs/` recursion (loop / sandbox-escape guard). - Cap recursion at `MAX_DEPTH = 8`. - Sort `readdirSync` output for deterministic generated content across platforms (CI-determinism, no more "works on my Mac" drift). - Prune orphan `*.generated.ts` files when a snippet is renamed or removed (otherwise rename leaves a tracked stale file behind, which `snippets:check` still treats as drift-free because it just looks for new diffs). - Render generated file with a `// @ts-nocheck` pragma so the body (verbatim untyped JS from the docs snippet) doesn't block the TypeScript compile when consumed. - Extra blank line between banner and content for grep-friendliness. **CI ordering** (A's should-fix): `npm run snippets:check` now runs AFTER `npm run lint` in `.github/workflows/lint.yml` — an extractor crash on a future malformed marker no longer masks a real lint failure as a docs-tooling failure. **tsconfig.test.json**: `test-utils` added to `include` so the generated snippet IS in the test build graph (combined with `@ts-nocheck`, the file is reachable for `import` without breaking compile). Together these close findings B5 (duplicate-reply pattern is documented in memory as part of the same review), plus all the should-fix items A and C surfaced on the codegen MVP. The follow-up A path (expand docs adapter so the test inline copy can be replaced by an `import` from `test-utils/snippets/currency-adapter.generated`) remains a product decision for Sequba and is tracked in [[project_hf_24_followups]].
sequba
reviewed
May 26, 2026
Comment on lines
+219
to
+221
| #### What is an LCID tag? | ||
|
|
||
| Excel can mark a currency format with a [Microsoft Locale Identifier](https://learn.microsoft.com/openspecs/windows_protocols/ms-lcid) (LCID) so the symbol carries locale context. The syntax is `[$SYMBOL-LCID]` followed by the number template — for example `[$zł-415] #,##0.00` means *"Polish złoty, hex LCID `415` = `pl-PL`"*, and `[$€-2] #,##0.00` means *"euro, generic"*. The adapter above parses the LCID to pick the matching `Intl.NumberFormat` locale and ISO 4217 currency code. |
Contributor
There was a problem hiding this comment.
How does Excel handle LCID tags?
Collaborator
Author
There was a problem hiding this comment.
Excel resolves LCID tags natively — no extra configuration is required. For example, [$€-2] #,##0.00 automatically uses European grouping and decimal separators and produces 1.234,50 €; [$zł-415] #,##0.00 uses pl-PL and produces 1 234,50 zł. HyperFormula's built-in formatter does not resolve LCID tags; the adapter replicates that behavior via Intl.NumberFormat.
Added a clarifying paragraph to the "What is an LCID tag?" section in 8cbb139d4.
Comment on lines
+63
to
+64
| "snippets:extract": "node script/extract-doc-snippets.js", | ||
| "snippets:check": "node script/extract-doc-snippets.js && git diff --exit-code -- test-utils/snippets/", |
Contributor
There was a problem hiding this comment.
What is the purpuse of these scripts?
Collaborator
Author
There was a problem hiding this comment.
These two scripts maintain sync between the docs code examples and the generated test utilities in test-utils/snippets/:
snippets:extract— scans markdown docs for<!-- snippet:NAME --> … <!-- /snippet:NAME -->fences and writes each extracted block totest-utils/snippets/<NAME>.generated.ts, so the same code that appears in the docs can be imported directly in tests.snippets:check— runs the same extraction, then asserts viagit diff --exit-codethat the generated files match what is committed. Used in CI to ensure docs snippets and generated test utilities never drift apart.
| * The INDEX function doesn't support returning whole rows or columns of the source range – it always returns the contents of a single cell. | ||
| * The FILTER function accepts either single rows of equal width or single columns of equal height. In other words, all arrays passed to the FILTER function must have equal dimensions, and at least one of those dimensions must be 1. | ||
| * Array-producing functions (e.g., SEQUENCE, FILTER) require their output dimensions to be determinable at parse time. Passing cell references or formulas as dimension arguments (e.g., `=SEQUENCE(A1)`) results in a `#VALUE!` error, because the output size cannot be resolved before evaluation. | ||
| * The TEXT function does not accept embedded double-quote literals in the format string (e.g., `=TEXT(A1, "#,##0.00 ""zł""")` fails at parse time). Use Excel's LCID-tagged form — `[$SYMBOL-LCID]` where LCID is a hex [Microsoft Locale ID](https://learn.microsoft.com/openspecs/windows_protocols/ms-lcid), e.g. `[$zł-415] #,##0.00` for Polish złoty — or supply a custom [`stringifyCurrency`](currency-handling.md) callback that handles such formats outside the parser. For locale-specific patterns like the Polish `"1234,50 zł"` (decimal comma), the callback is required because the built-in number formatter always emits `.` as the decimal separator. |
Contributor
There was a problem hiding this comment.
How does Excel handle formats like =TEXT(A1, "#,##0.00 ""zł""")?
Collaborator
Author
There was a problem hiding this comment.
In Excel, "" inside a format string is an escape sequence for a literal " character — so #,##0.00 ""zł"" is equivalent to #,##0.00 "zł" (a number pattern with zł as a quoted literal suffix) and =TEXT(1234.5, "#,##0.00 ""zł""") returns "1,234.50 zł". HyperFormula's parser does not support this escape sequence and fails at parse time.
Updated the limitation entry to document what Excel produces before stating the HF restriction, in 8cbb139d4.
| @@ -0,0 +1,235 @@ | |||
| # Currency handling | |||
Contributor
There was a problem hiding this comment.
Nice, solid guide explaining everything very well 👍
Per code review — TypeScript signature already declares parameter
and return types, so {type} brackets in JSDoc are redundant noise
and inconsistent with the sibling exported functions in this file
(defaultStringifyDuration, defaultStringifyDateTime have no JSDoc
type tags).
…ngify callbacks
…claim, mention U+202F NBSP)
… entry, embedded-quote nuance, adapter guard)
- Add typed contract signature block at top - Add Minimal example subsection (3-line callback for fresh-user contract) - Add Default behavior subsection (explains defaultStringifyCurrency) - Add Error behavior subsection (callback exception propagation) - Add MS-LCID specification link in adapter intro - Drop trailing-quote rule from CURRENCY_RULES (not callable from TEXT) - Move NBSP tip below console.log output (was between config and output)
…vent letter-format hijack The previous order (DateTime -> Duration -> Currency) let parseForDateTimeFormat greedily match characters D, M, S, Y, H inside currency format strings. Formats like '[$USD-409] #,##0.00' or 'USD #,##0.00' were converted to '[$US9-409] #,##0.00' before the user-supplied stringifyCurrency callback could intercept them. Currency dispatch now runs first. The default callback returns undefined for every input, so the existing date/time/duration/number-format chain is preserved bit-for-bit when stringifyCurrency is not set. Found by Codex review (codex-cli 0.130.0, base develop, max effort).
defaultStringifyDateTime now returns undefined when formatArg contains Excel's LCID-tagged currency notation [$SYMBOL-LCID]. Without this guard, parseForDateTimeFormat greedily consumed D/M/S/Y/H letters inside the currency code, mangling output even when a user-supplied stringifyCurrency callback returned undefined for opt-out. Before: TEXT(100, '[$USD-409] #,##0.00') with partial callback -> '[$US9-409] #,##0.00' (D->9 mangle) After: TEXT(100, '[$USD-409] #,##0.00') with partial callback -> '[$USD-41009] #,##0.00' (USD preserved, falls through to numberFormat) Excel never uses [$...] for date formats, so the guard is unambiguous. Found by Codex re-review (after first dispatch reorder fix d119b4c).
… date locale) Codex re-review identified that the prior LCID guard (introduced in d496e30) over-matched Excel's locale-only modifier syntax `[$-LCID]` used in date and time formats (e.g. `[$-409]dd/mm/yyyy`), incorrectly skipping date dispatch and falling through to numberFormat. The guard regex now requires a non-empty SYMBOL portion between `[$` and the dash. Currency tags (`[$USD-409]`, `[$€-2]`, `[$zł-415]`) continue to skip date dispatch as intended; locale-only modifiers (`[$-409]`, `[$-F800]`) flow through to parseForDateTimeFormat as before. Also softens the 'bit-for-bit preserved' doc claim: for LCID-tagged currency formats without a callback, output now goes through numberFormat (best-effort) instead of the pre-existing date-parser hijack. Setting stringifyCurrency remains the recommended path.
…tency) Bugbot identified that the LCID-tagged currency guard added to defaultStringifyDateTime (b7c61a5) was missing from its sibling defaultStringifyDuration. Currency symbols containing duration-token letters (H in CHF/HUF, M in AMD/HMD) were interpreted as time tokens when a stringifyCurrency callback returned undefined. Applies the same regex `/\[\$[^\-\]]+-/` guard in identical position to preserve sibling parity with defaultStringifyDateTime.
Bugbot Low: previous comment 'preserving the existing dispatch path bit-for-bit when stringifyCurrency is not set' was inaccurate after the LCID guards landed in defaultStringifyDateTime/Duration. For non-currency formats bit-for-bit holds; for LCID-tagged currency formats output now falls through to numberFormat instead of being mangled by the date parser — a deliberate improvement, not a preservation. Comment reworded to acknowledge this.
Public branch merged upstream/develop in d77d5a6 (bringing HF-85 D-function code). Tests-repo branch merged origin/develop in 354b872 (bringing HF-85 D-function tests). CI clones tests-repo by matching branch name, so this empty commit re-runs the full matrix with the updated tests checkout. Should resolve the codecov/project drop (was -1.40% because D-function code shipped without matching tests in the same branch namespace).
The previous wording suggested that the built-in number formatter handles `$#,##0.00` via the default dispatch path. Sandbox audit showed the built-in numberFormat actually fails on any format that includes the comma thousands separator: TEXT(1234.5, "$#,##0.00") -> "$1235,##0.00" (not "$1,234.50") The intro paragraph now lists only the formats that genuinely work without a callback (`$0.00`, `$0`, `$#.00`) and explicitly calls out the broken cases (`$#,##0.00`, LCID-tagged, accounting two-section). The Default behavior subsection gains a side-by-side comparison table (without callback / with adapter / Excel) and a recommendation to set `stringifyCurrency` for any application showing currency to users. Docs-only change. No source or test impact.
Addresses review feedback on PR #1665: - Extract Currency integration section from date-and-time-handling.md into a new top-level guide currency-handling.md so the topic stands on its own and is reachable from the sidebar. - Open the guide with a positive framing: out-of-the-box support for simple $-prefixed formats, with stringifyCurrency as the additive extension point for richer patterns. Lead with a default-behavior example before introducing the callback. - known-limitations: explain what an LCID tag is inline and clarify that the Polish '1234,56 zł' (decimal-comma) pattern requires the stringifyCurrency callback because the built-in number formatter always emits '.' as the decimal separator. - list-of-differences: drop the TEXT-formatting paragraphs that were duplicating the new guide; replace with a one-liner pointing at stringifyDateTime + stringifyCurrency for full TEXT coverage. - compatibility-with-microsoft-excel and compatibility-with-google-sheets: add a 'TEXT function formats' subsection noting that both callbacks together cover the full TEXT format range. - Update ConfigParams.ts JSDoc cross-reference to point at the new currency-handling guide. Regenerated docs/api artifacts (gitignored) follow automatically via typedoc on docs:build.
The previous push uploaded commit f1eb4ef to upstream but the pull_request:synchronize event did not fire workflow runs — only the lightweight bot checks (Cursor Bugbot, Netlify rules) attached. This empty commit forces a fresh synchronize so the full matrix (Lint, Test, Build on various envs, Performance, Security, CodeQL, Build docs) runs against the new docs reorganization.
Address Kuba's review feedback on the input-vs-output framing question: - currency-handling.md becomes the single authority for currency: both the input-parsing mechanism (currencySymbol) and the output formatting mechanism (stringifyCurrency callback) live together, with an explicit framing of their independence at the top of the guide. - Add a 'Currency input' section showing currencySymbol with a Polish złoty example, the prefix/suffix detection, and the NUMBER_CURRENCY detailed type that input parsing produces. - Reorganize the rest of the guide as 'Currency output' so the structure mirrors the framing — default behavior, custom callback, reference table, Intl.NumberFormat adapter, LCID explainer, library swap. - i18n-features.md drops its standalone 'Currency symbol' subsection and links to the new guide instead, removing the duplication.
Address three nits surfaced by the final fresh-eyes review: - ConfigParams.ts: stringifyCurrency was filed under @category 'Date and Time' (copied from sibling stringifyDateTime/Duration), placing a currency-formatting option under the Date and Time TypeDoc nav group. Move it to @category 'Number' to match where currencySymbol already lives, so currency-related config clusters together in generated API docs. - ConfigParams.ts: expand the stringifyCurrency JSDoc to state plainly that the formatter calls the callback for every format string that reaches it, not only currency-shaped ones, and that returning undefined is the opt-out path for unsupported formats. IDE users reading the contract no longer have to infer this from the guide. - format.ts: hoist the LCID-tagged currency-guard regex into a module-level const (LCID_CURRENCY_TAG) shared by defaultStringifyDateTime and defaultStringifyDuration. Avoids re-instantiating the same RegExp on every format() call (TEXT can be invoked thousands of times per recalc), and documents at the declaration site why the pattern is intentionally unanchored — Excel does not mix date/time tokens with currency tags in a single format string, so a mid-string match cannot misclassify a legitimate composite.
…tent) Replaces the inline rationale block with proper JSDoc on `defaultStringifyDateTime` and `defaultStringifyDuration`. Captures: - The historical pre-HF-24 mis-formatting (`[$USD-409] #,##0.00` → `[$US9-409] #,##0.00` because `D` was treated as a day token). - Why the guard is the deliberate correction, not a regression — every non-currency format remains bit-for-bit compatible. - The `[$SYMBOL-LCID]` vs `[$-LCID]` distinction the regex enforces. - The dispatch contract (`undefined` = defer to next handler) so future callers reason about the fall-through path explicitly. Addresses the design-intent angle raised by Bugbot wave 2 (low-severity inline at format.ts:196). No behavioural change.
Adds the build infrastructure to keep the `customStringifyCurrency` adapter (and any future documented snippet) in lockstep between docs/guide/currency-handling.md and downstream test/utility code. Why this exists: The O5 pattern that HF-24 itself surfaced (Bugbot wave 1, commit b81d4af) was a docs adapter gaining a `typeof !== 'string'` guard while an inline copy in test/.../function-text.spec.ts stayed out of date — edge tests then crashed on `null.split(';')`. Manual "synchronize on every edit" doesn't survive contact with reality. What this lands: - `script/extract-doc-snippets.js` — walks docs/**/*.md, extracts every `<!-- snippet:NAME -->` ... `<!-- /snippet:NAME -->` block (fenced code inside), writes verbatim to test-utils/snippets/<NAME>.generated.ts with a header banner. Zero npm deps; runs in the same Node we use for `compile`. - `npm run snippets:extract` — regenerates all snippets. - `npm run snippets:check` — extracts + `git diff --exit-code` on the output dir, so CI can gate against drift in a single command. - docs/guide/currency-handling.md — adapter wrapped in `snippet:currency-adapter` markers (cosmetic-only edit; the rendered docs are unchanged because VuePress treats `<!-- ... -->` as a comment). - test-utils/snippets/currency-adapter.generated.ts — initial extraction committed so downstream callers can `import { customStringifyCurrency }` from a stable path. What's deferred (follow-up PR in hyperformula-tests): - Switch test/hyperformula-tests/unit/interpreter/function-text.spec.ts to `import { customStringifyCurrency } from '../../../../<repo>/test-utils/snippets/currency-adapter.generated'` instead of re-defining the adapter inline. That closes the fixture-flagged O5 finding for good; doing it in this PR would mix cross-repo changes and complicate review. - CI integration: a `npm run snippets:check` step in the lint/test workflow. Trivial follow-up once the marker convention lands. Single-source-of-truth direction: documentation. Edit the markdown snippet, run `npm run snippets:extract`, commit the regenerated file — or let CI catch the drift.
Runs `npm run snippets:check` (= extract + `git diff --exit-code` on `test-utils/snippets/`) before the linter. Fails the lint job — same gate as eslint — when a documented snippet has drifted from the committed generated file. Closes the feedback loop the codegen infrastructure was built for: docs become a load-bearing source of truth, not a parallel artifact that drifts silently. Companion to 6a441b3 (the codegen MVP); together these mean any future edit to `docs/guide/currency-handling.md`'s adapter snippet must either be regenerated locally or the CI will block the merge.
Addresses the must-fix + should-fix findings from the parallel code-review-quality (A) and sherlock-review (C) passes on PR #1665. **P0 — `tryAccountingFormat` sign-loss bug** (A's only must-fix bug): docs adapter `customStringifyCurrency` in `currency-handling.md` mis-rendered negative values when the negative section was plain `$#,##0.00` (no parens). `Math.abs(value)` was formatted without ever adding the `-` prefix, so `value = -1234.5` against format `"$#,##0.00;$#,##0.00"` returned `$1,234.50` (positive-looking) instead of `-$1,234.50`. New branch: `parenMatch ? "(" + formatted + ")" : "-" + formatted`. Regression test for this path landed in hyperformula-tests `85979a0`. **`extract-doc-snippets.js` hardening** (C's main concerns): - Detect malformed `<!--snippet:NAME-->` markers (no spaces around the body) and fail loudly instead of silently skipping. Pre-fix, a typo in a docs author's marker meant the snippet was silently dropped from `test-utils/snippets/` while `snippets:check` still passed — drift undetected. - Skip symbolic links during `docs/` recursion (loop / sandbox-escape guard). - Cap recursion at `MAX_DEPTH = 8`. - Sort `readdirSync` output for deterministic generated content across platforms (CI-determinism, no more "works on my Mac" drift). - Prune orphan `*.generated.ts` files when a snippet is renamed or removed (otherwise rename leaves a tracked stale file behind, which `snippets:check` still treats as drift-free because it just looks for new diffs). - Render generated file with a `// @ts-nocheck` pragma so the body (verbatim untyped JS from the docs snippet) doesn't block the TypeScript compile when consumed. - Extra blank line between banner and content for grep-friendliness. **CI ordering** (A's should-fix): `npm run snippets:check` now runs AFTER `npm run lint` in `.github/workflows/lint.yml` — an extractor crash on a future malformed marker no longer masks a real lint failure as a docs-tooling failure. **tsconfig.test.json**: `test-utils` added to `include` so the generated snippet IS in the test build graph (combined with `@ts-nocheck`, the file is reachable for `import` without breaking compile). Together these close findings B5 (duplicate-reply pattern is documented in memory as part of the same review), plus all the should-fix items A and C surfaced on the codegen MVP. The follow-up A path (expand docs adapter so the test inline copy can be replaced by an `import` from `test-utils/snippets/currency-adapter.generated`) remains a product decision for Sequba and is tracked in [[project_hf_24_followups]].
The browser-tests + unit-tests CI runs on HEAD `83dafd163` failed against the previous tests-repo HEAD `85979a0` whose `function-text.spec.ts` letter-hijack assertion was too strict. Fixed in hyperformula-tests:feature/hf-24-stringify-currency commit `ed38a4f` (reverts to checking only `[$SYMBOL-` boundary, which the date-parser hijack would destroy, instead of the full `[$SYMBOL-LCID]` prefix that gets re-shaped by the fallback number formatter). Empty commit triggers CI re-fetch of tests-repo by branch name — same pattern as the standard cross-repo iteration loop ([[reference_cross_repo_ci]]). No HF code/docs changes in this commit.
…hes Excel
Reverts the explicit `-` prefix branch introduced for the plain-negative
section case in `83dafd163`. Empirical Excel 2021 verification on
2026-05-25 against format `$#,##0.00;$#,##0.00` with value -1234.5
renders `$1 234.50` (no minus) — NOT `-$1 234.50` as the original
code-review reasoning claimed.
Excel format-string spec: an explicit two-section format
`<positive>;<negative>` is honoured AS-IS. Auto-sign only applies to
single-section formats. The pre-fix adapter logic
`return isNegative && parenMatch ? '(' + formatted + ')' : formatted`
mirrors Excel exactly across all four (pos/neg) × (paren/plain) cases.
The earlier "fix" introduced a regression for the negative×plain case.
Updated the inline comment to call out the Excel-spec rationale so the
behaviour is no longer misread as an oversight by future readers.
Regenerated `test-utils/snippets/currency-adapter.generated.ts` via
`npm run snippets:extract` to keep the source-of-truth in sync.
Companion test revert + regression-test reframing in hyperformula-tests
commit `5e8226c` on `feature/hf-24-stringify-currency`. Closes-out the
false-positive must-fix that consumed two commits and a Slack-Q draft
cycle.
Empty commit to trigger fetch-tests of the updated
`hyperformula-tests:feature/hf-24-stringify-currency` HEAD `87e72b5`,
which drops the inline `customStringifyCurrency` adapter copy from
`function-text.spec.ts` (~100 LOC including the unused symbol-suffix
3rd rule + localeBySymbol map) and consumes the docs snippet directly
via `require('../../../../test-utils/snippets/currency-adapter.generated')`.
This closes the codegen MVP loop end-to-end on HF-24:
docs/guide/currency-handling.md → snippets:extract → test-utils/snippets/
currency-adapter.generated.ts → required by function-text.spec.ts.
snippets:check CI gate prevents drift on either side from this point on.
Expected effect on next `prep audit 1665`: O5 (source-of-truth duplication)
detection count drops from 13 to ≤2 — only the two `hfInstance` FP
detections from compatibility-* docs remain (canonical naming convention
across the codebase, not real duplication; tracked as follow-up D in
project_hf_24_followups memory).
Three concerns surfaced by `prep ultra` (Opus fresh-eyes review) and the A+C parallel review on HEAD `c3b65382a`. Addressing all three before flip-to-review. **L1 — Docs adapter regex divergence from core guard** (Ultra Low): The first `CURRENCY_RULES` pattern in `docs/guide/currency-handling.md` used `[^\-\]]*` (zero-or-more) for the SYMBOL portion, where the production `LCID_CURRENCY_TAG` in `src/format/format.ts:26` uses `[^\-\]]+` (one-or-more). The looser docs regex would cause copy-paste users to mis-classify Excel's locale-only modifier `[$-409]` (used on date/time formats) as a currency format and route it through the LCID table, producing silent en-US currency formatting on what is meant to be a Polish/English date format. Tightened to `+` and added an explanatory comment so future readers know why the constraint is one-or-more. Regenerated `test-utils/snippets/currency-adapter.generated.ts` via `npm run snippets:extract` so the codegen artifact picks up the fix. **M1 — CHANGELOG framing** (Ultra Medium): The existing `Added a stringifyCurrency config option` line is purely additive framing. But the LCID guards in `defaultStringifyDateTime` and `defaultStringifyDuration` change observable `TEXT` output for **every** LCID-tagged currency format (`[$USD-409] #,##0.00`, `[$€-2] #,##0.00`, etc.) regardless of whether a `stringifyCurrency` callback is configured. Pre-fix: mangled by the date parser (`[$US9-409]`). Post-fix: falls through to `numberFormat`. Strictly an improvement, but upgraders who snapshot-test `TEXT()` output should know to expect it. Added a `Fixed` entry describing the behavioural correction with concrete before/after. **Lint blocker** (A+C parallel review): Added `test-utils/snippets/` to `.eslintignore`. Generated adapter content carries `@ts-nocheck` (necessary — JS body without TypeScript annotations) and has implicit-`any` operands (`'-' + match[2]`) that ESLint's `@typescript-eslint/ban-ts-comment` + `no-unsafe-*` rules flagged as errors on HEAD `c3b65382a` lint(22). Auto-generated content shouldn't be linted; `script/`, `commonjs/`, `dist/`, etc. were already ignored — extending the same policy to `test-utils/snippets/` is consistent. Companion fix on hyperformula-tests `feature/hf-24-stringify-currency` commit `7663f5c` cleans up pre-existing lint errors that became visible after PR #1672 extended lint scope to tests-repo (`Array<T>` → `T[]`, `opt_out` → `optOut`, `as unknown as string` → `as string`). Local verification (2026-05-25): - `npm run compile` clean - `npx tsc -p tsconfig.test.json --noEmit` clean - `npm run snippets:check` exit 0 (generated byte-identical post-regen) - `eslint` on modified files: 0 errors - `npm run test:jest -- --testPathPattern function-text` reports 53/53 pass
Two cosmetic nits surfaced by Marcin's line-by-line file review on 2026-05-25, fixed in one commit since they touch the same iteration. **Nit 1 — `docs/guide/currency-handling.md:89` post-HF-24 dispatch wording** The line previously said "Dates, durations, and unrecognized formats continue through HyperFormula's *existing* dispatch chain." Accurate pre-HF-24, slightly stale post-HF-24 because `stringifyCurrency` now runs FIRST in the dispatch order (the very change this PR ships). Reworded to: "For any format the callback opts out of, HyperFormula proceeds to the next handler in the dispatch chain: the default date / duration formatters, then the built-in number formatter, and finally the raw format string if nothing matched." Concrete and forward-correct. **Nit 2 — Block-level comments leaked into generated test artifact** The generated `test-utils/snippets/currency-adapter.generated.ts` is a byte-for-byte copy of the docs snippet, which means editorial comments useful for a docs reader (e.g. `// Minimal Excel-format-string → Intl.NumberFormat adapter.`, `// [$SYMBOL-LCID] — Excel's locale-tagged currency.`) flow through into the test artifact unchanged. They add verbosity without value for the downstream `import` consumer (jest). Added `stripBlockComments(code)` to `script/extract-doc-snippets.js` that drops pure `// …` lines (entire line is whitespace + comment) and collapses the resulting runs of blank lines. Trailing comments after live code are deliberately left alone — stripping those safely needs a JS tokenizer (to skip `'https://…'`-style string literals), and the block-level strip alone reduces the generated file from 85 to 71 lines on the current snippet, which addresses the noise concern. Regenerated the artifact via `npm run snippets:extract` so the `snippets:check` CI gate stays clean. Verification (2026-05-25): - `npm run compile` clean - `npx tsc -p tsconfig.test.json --noEmit` clean - `npm run snippets:check` exit 0 (regen produces committed bytes) - `npm run test:jest -- --testPathPattern function-text` reports 53/53 pass (no test depends on the stripped comments)
Two findings from the brutal-honesty self-review surfaced after CI went green: **Finding 2 — CHANGELOG entries under a RELEASED version.** Per Sequba's policy stated on PR #1645 (2026-04-03): *"Release notes are updated later (during the release). During the day-to-day development we only need to keep the CHANGELOG up-to-date"* — meaning in-progress PRs add entries to `[Unreleased]`, not to the most recent shipped version. `[3.3.0]` is dated 2026-05-20 (frozen, released). Moving both the existing `Added stringifyCurrency` line (carried over from an earlier commit in this branch) and the new `Fixed` entry for the LCID guard regression from `[3.3.0]` to `[Unreleased]`. Editing released sections rewrites history; that's what `[Unreleased]` is for. **Finding 1a — `byte-identical` codegen contract claim was stale.** The first codegen commit (`6a441b310`) claimed "byte-identical regen byte-identical with the docs snippet" and the script docstring said "writes the code (verbatim)". The recent `stripBlockComments` change made that no longer true: 40+ lines of editorial block comments present in the docs source are dropped from the generated artifact. The drift gate (`snippets:check`) is unchanged (`generated matches its own regeneration`) but the **contract description was a lie**. Rewrote the script docstring to say what actually happens: generated is "functionally equivalent, not byte-identical"; what's gated is "generated matches its own regeneration", not "generated matches docs body". Future engineers reading the script no longer hit the "wait, this doesn't match docs — is this a bug?" moment. No code-behaviour changes — both fixes are documentation accuracy.
handsontable/hyperformula-tests@6ebdbf8 moves the eslint-disable-next-line annotation inside the try-block so it covers the actual require() call (was on the const declaration two lines up, which left the require unprotected and tripped lint on b018aed CI). This empty commit re-runs the cross-repo CI matrix on the same main-repo HEAD against the updated tests-repo HEAD. No source changes here.
handsontable/hyperformula-tests@8c87bbe defers the generated-adapter access into a beforeAll hook so the standalone-clone fallback path no longer crashes Karma/Jasmine. Empirically verified by hiding the generated file and re-running Jest: 20 passed + 53 skipped, no TypeError. Re-runs the cross-repo CI matrix on the same HF-main HEAD against the updated tests-repo HEAD. No source changes here.
…te escaping, list-of-differences wording - list-of-differences.md: reword TEXT function row to Kuba's suggested phrasing - currency-handling.md: add paragraph explaining Excel's native LCID resolution - known-limitations.md: explain Excel's double-quote escape behavior before stating HF limitation Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Harness 04-link-validation flagged two relative links missing the .md extension: localizing-functions → localizing-functions.md and custom-functions → custom-functions.md. VuePress resolves both forms at build time, but adding .md is consistent with all other links in the file and removes the harness warning.
marcin-kordas-hoc
force-pushed
the
feature/hf-24-stringify-currency
branch
from
1bede82 to
cb2fa8f
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #1665 +/- ##
========================================
Coverage 97.16% 97.16%
========================================
Files 176 176
Lines 15322 15333 +11
Branches 3356 3359 +3
========================================
+ Hits 14887 14898 +11
Misses 427 427
Partials 8 8
🚀 New features to boost your workflow:
|
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
3 participants
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.
Summary
Adds a
stringifyCurrencyconfig option mirroring the existingstringifyDateTime/stringifyDurationcallbacks. When set, theTEXTfunction consults the callback before falling through to the built-in number formatter, so users can plug in locale-aware currency formatting (for example viaIntl.NumberFormator a third-party library) without bringing currency data into the HyperFormula core.The default implementation returns
undefinedso existing TEXT behavior is preserved bit-for-bit for non-LCID-tagged formats. LCID-tagged currency strings ([$SYMBOL-LCID]) now correctly skip the date/time parser (see Fixed in CHANGELOG); that is a separate behavioral fix, not a regression.Linked
agents/hyperformula/docs/specs/2026-04-21-hf-24-currency-in-text.mdagents/hyperformula/docs/specs/2026-04-24-hf-24-tech-rationale.mdagents/hyperformula/docs/specs/2026-04-27-hf-24-stringify-currency-plan.mdTests
Tests added in the matching
feature/hf-24-stringify-currencybranch ofhandsontable/hyperformula-tests. Coverage:undefinedundefined) → fall-through tonumberFormatstringifyCurrencyIntl.NumberFormatadapter (USD shorthand, EUR via LCID, JPY via LCID, PLN via LCID, accounting two-section), plus a fall-through case demonstrating opt-outNotes
#,##0.00 "zł"(trailing quoted symbol). HF's formula parser does not accept embedded quotes inside TEXT format strings, so the docs example and the corresponding test were swapped to use[$zł-415] #,##0.00(LCID-tagged symbol). The adapter still recognizes the trailing-quote pattern for users invoking the callback outside HyperFormula.'1.234,50 €'(symbol-trailing) for[$€-2]and'￥1,235'(full-width yen sign, no space) for[$¥-411]because that is whatIntl.NumberFormat('de-DE'/'ja-JP', ...)actually produces on modern Node ICU. NBSP normalization in tests covers both\u00A0and\u202Fvariants for ICU build robustness.[$SYMBOL]boundary: the example regex requires the-LCIDsegment. A bare[$USD]pattern is not handled by the adapter and falls through to the built-innumberFormat, whose handling of[$...]in HyperFormula is implementation-defined. Testdocs adapter does not handle [$SYMBOL] without LCID segmentdocuments the boundary.docs/guide/built-in-functions.md; (b)docs/guide/i18n-features.mdreplaces a stalecurrencySymbolcode example with a cross-reference; (c)tsconfig.test.jsonaddstest-utilstoinclude; (d)docs/guide/known-limitations.md— new bullet for the TEXT embedded-quote limitation (can't use""escape in format strings; use LCID-tagged form orstringifyCurrencycallback); (e)docs/guide/date-and-time-handling.md— adds cross-reference paragraph tocurrency-handling.md; (f)docs/guide/list-of-differences.md— updates the "No currency formatting in TEXT" row with callback instructions; (g).eslintignore— addstest-utils/snippetsexclusion; (h)test-utils/snippets/currency-adapter.generated.ts— generated snapshot committed intentionally; it is the source-of-truth for snippet correctness tests and not a build artifact. Regenerated vianpm run snippets:extract. (i)script/extract-doc-snippets.js(256 LOC) — new codegen script that walks the docs for<!-- snippet:NAME -->markers and syncs them intotest-utils/snippets/*.generated.ts. Heavy for one snippet but designed for future docs expansion; see inline docstring for design rationale.456adddff= develop with HF-85 DatabasePlugin). HF-24's runtime impact is one extra dispatcher call informat()perTEXTinvocation, which the Sheet A/B/T benchmarks don't exercise. The variance is most likely benchmark noise or HF-85 import overhead carried in via the develop merge, not HF-24-specific.Maybe<T>in callback types:stringifyCurrency,stringifyDateTime, andstringifyDurationall declare return type asMaybe<string>(=string | undefined). This is consistent public API surface shared by all three siblings; the alias is re-exported from HyperFormula's type definitions. Changing onlystringifyCurrencyto barestring | undefinedwould create asymmetry. A follow-up can align all three if desired.Test plan
handsontable/hyperformulaPRhandsontable/hyperformula-testsPR (matching branch)vuepress build docsreturns EXIT 0 with 215 sitemap entries; newcurrency-handling.mdguide renders with theCurrency input+Currency outputsections (sidebar wired under Internationalization) per Kuba's review feedbackPrivate tests PR: handsontable/hyperformula-tests#10
Note
Medium Risk
Changes
TEXTformatting dispatch and fixes LCID-tagged currency behavior (intentional vs pre-fix mangling); default callback is a no-op, but custom formatters and edge cases need regression coverage in hyperformula-tests.Overview
Adds
stringifyCurrencyas a config callback (likestringifyDateTime/stringifyDuration) soTEXTcan delegate currency formatting before the built-in number formatter; the default implementation always returnsundefined, preserving prior behavior for ordinary formats.TEXTdispatch now runs the currency callback first.defaultStringifyDateTimeanddefaultStringifyDurationbail out when the format contains an LCID currency tag ([$SYMBOL-LCID]), fixing mangled output (e.g.[$USD-409]→[$US9-409]) when letters in the symbol were parsed as date/time tokens.Documentation adds
currency-handling.md, wires it in the VuePress sidebar, and updates compatibility/limitations guides. CI gainssnippets:checkplusscript/extract-doc-snippets.js, which syncs<!-- snippet:... -->blocks from docs intotest-utils/snippets/*.generated.ts(e.g. theIntl.NumberFormatadapter used by tests).Reviewed by Cursor Bugbot for commit 69d5916. Bugbot is set up for automated code reviews on this repo. Configure here.