HF-24: add stringifyCurrency config callback for TEXT#1665
Open
marcin-kordas-hoc wants to merge 43 commits into
Open
HF-24: add stringifyCurrency config callback for TEXT#1665marcin-kordas-hoc wants to merge 43 commits into
marcin-kordas-hoc wants to merge 43 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 (260f316) 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
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
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).
…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
sequba
requested changes
Jun 2, 2026
sequba
left a comment
sequba
left a comment
Contributor
There was a problem hiding this comment.
Ok, now that I understand the purpose of snippets:extract and snippets:check, I can finish the review. There are a few more cosmetic things to fix, but we're almost there.
Addresses review on #1665. Snippets: - Stop committing test-utils/snippets/*.generated.ts; regenerate them from the docs before every test run instead (docs stay the single source of truth, no duplicated snippet code in the repo). - package.json: drop snippets:check (the git-diff drift gate); add pretest:ci, pretest:jest and pretest:browser hooks that run snippets:extract, so the artifacts are generated by the CI test jobs and locally before jest/karma. - .gitignore: ignore test-utils/snippets/*.generated.ts. - lint.yml: remove the snippets drift check (no longer part of linting). - extract-doc-snippets.js: update docstring/banner to the regenerate-on-test model. Docs/changelog: - CHANGELOG: move the LCID-tagged currency-format entry to Added (new capability). - known-limitations.md, currency-handling.md: tighten wording.
The unit-tests and browser-tests jobs regenerate the doc snippets via the `pretest:ci` / `pretest:browser` npm hooks before running the tests; add a comment at each "Run tests" step so the generation is discoverable in the workflow without duplicating it as a separate step.
sequba
requested changes
Jun 8, 2026
sequba
left a comment
sequba
left a comment
Contributor
There was a problem hiding this comment.
Good. The only thing is that the pretest:* scripts are never run. Look at my suggestions.
… pretest hooks Address Kuba's review on #1665 (inline threads + 2026-06-09 dev sync): the `pretest:jest` / `pretest:ci` / `pretest:browser` npm hooks did not reliably run `snippets:extract` before the tests — the top-level `test` script invokes the test scripts via `npm-run-all`, which does not fire npm `pre*` lifecycle hooks, so the generated snippet artifacts could be missing and tests would error. Per Kuba's suggestion, prepend `npm run snippets:extract &&` directly to `test:jest`, `test:ci`, and `test:browser` so generation is bulletproof on every invocation path (CI and local), and remove the redundant `pretest:*` hooks. - package.json: inline `snippets:extract` into test:jest / test:ci / test:browser; remove pretest:jest / pretest:ci / pretest:browser. - .github/workflows/test.yml: update the unit- and browser-test comments to reference the inline `test:*` mechanism instead of the removed pretest hooks. - CHANGELOG.md: shorten the LCID-tagged currency entry to Kuba's suggested one-liner. - .gitignore, script/extract-doc-snippets.js: refresh stale comments that referenced the removed `pretest:*` hooks. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
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 3390 +34
========================================
+ Hits 14887 14898 +11
- Misses 427 435 +8
+ Partials 8 0 -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 Added 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/*.generated.ts— generated from the docs and not committed; regenerated before every test run via thepretest:*npm hooks (snippets:extract) and git-ignored, so the docs stay the single source of truth. (i)script/extract-doc-snippets.js(257 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 public config and the core
TEXTformatting dispatch order; LCID-tagged formats behave differently than before (intentional fix), so integrators relying on old output should verify against their format strings.Overview
Adds
stringifyCurrencyonConfigParams/Config, mirroringstringifyDateTimeandstringifyDuration.TEXTnow calls it first informat(); the default always returnsundefinedso non-currency formats keep the same path, while custom callbacks can supply locale-aware currency output before the built-in number formatter.LCID-tagged currency strings (
[$SYMBOL-LCID] …) are no longer parsed as date/time: sharedLCID_CURRENCY_TAGguards indefaultStringifyDateTime/defaultStringifyDurationplus running currency dispatch first fix mangled output (e.g.[$USD-409] #,##0.00).Documentation adds
currency-handling.md(sidebar + Excel/Sheets compatibility notes), changelog entries, and TEXT limitation notes.script/extract-doc-snippets.jsandsnippets:extractwired intotest:jest/test:ci/test:browserso doc<!-- snippet:NAME -->blocks generate gitignoredtest-utils/snippets/*.generated.tsfor tests.Reviewed by Cursor Bugbot for commit 260f316. Bugbot is set up for automated code reviews on this repo. Configure here.