HF-24: add stringifyCurrency config callback for TEXT#1665
Open
marcin-kordas-hoc wants to merge 38 commits into
Open
HF-24: add stringifyCurrency config callback for TEXT#1665marcin-kordas-hoc wants to merge 38 commits into
marcin-kordas-hoc wants to merge 38 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 (b555d08) vs base (508d78f) |
marcin-kordas-hoc
force-pushed
the
feature/hf-24-stringify-currency
branch
from
0246ce0 to
09babfd
Compare
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.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
…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.
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.
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).
Contributor
|
Task linked: HF-85 Implement function DCOUNT |
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.
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.
|
|
||
| And now, HyperFormula recognizes these values as valid dates and can operate on them. | ||
|
|
||
| ## Currency integration |
Contributor
There was a problem hiding this comment.
This section should be made into a separate guide currency-handling
Collaborator
Author
There was a problem hiding this comment.
Done. The whole "Currency integration" section moved into a new top-level guide docs/guide/currency-handling.md, wired into the Internationalization sidebar section. date-and-time-handling.md keeps a single-paragraph pointer to the new guide. Fixed in f1eb4ef.
Collaborator
Author
There was a problem hiding this comment.
Refined per your follow-up on Slack: currency-handling.md is now the single authority for currency (both input parsing via currencySymbol and output formatting via stringifyCurrency), with explicit input-vs-output framing at the top. i18n-features.md drops its standalone Currency symbol subsection and links here instead — no duplication. Done in e175670.
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.
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. |
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.
…ringify-currency # Conflicts: # docs/guide/known-limitations.md
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.
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 175 175
Lines 15319 15330 +11
Branches 3356 3359 +3
========================================
+ Hits 14884 14895 +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.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.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.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
Touches core
TEXTformatting dispatch and changes default handling of LCID-tagged currency format strings (intentional fix); misconfigured callbacks could affect allTEXTinvocations that reachformat().Overview
Adds
stringifyCurrencyas a new config callback (same pattern asstringifyDateTime/stringifyDuration) soTEXTcan delegate currency formatting to user code; the default implementation always returnsundefined, so non-currency behavior stays on the existing path unless you opt in.format()dispatch now runs the currency callback first, then date/time/duration, then the built-in number formatter.defaultStringifyDateTimeanddefaultStringifyDurationskip formats containing Excel LCID currency tags ([$SYMBOL-LCID]), fixing mangled output (e.g.[$USD-409]→[$US9-409]) when the date/time parser treated letters in the symbol as date tokens.Docs add a Currency handling guide (with an extractable
Intl.NumberFormatadapter snippet), sidebar and compatibility cross-links, and changelog entries. Tooling:script/extract-doc-snippets.js,npm run snippets:checkin CI, and generatedtest-utils/snippets/currency-adapter.generated.tsso tests import the same example as the docs.Reviewed by Cursor Bugbot for commit b555d08. Bugbot is set up for automated code reviews on this repo. Configure here.