Skip to content

feat: update default number formats to trim insignificant trailing zeros#10782

Merged
vasilev-alex merged 2 commits intomasterfrom
change-default-number-format
Apr 30, 2026
Merged

feat: update default number formats to trim insignificant trailing zeros#10782
vasilev-alex merged 2 commits intomasterfrom
change-default-number-format

Conversation

@vasilev-alex
Copy link
Copy Markdown
Member

No description provided.

@vasilev-alex vasilev-alex requested review from a team as code owners April 30, 2026 04:45
@github-actions github-actions Bot added client:core Issues relating to the JavaScript client SDK javascript Pull requests that update Javascript code labels Apr 30, 2026
@claude
Copy link
Copy Markdown
Contributor

claude Bot commented Apr 30, 2026

Claude finished @vasilev-alex's task in 2m 9s —— View job


Code Review: Update default number formats to trim trailing zeros

  • Gather context and read changed files
  • Review code quality and correctness
  • Review tests and snapshots
  • Post review feedback

Summary

This PR adds the d3-format ~ (trim) modifier to all default and named numeric format specifiers, so that insignificant trailing zeros are stripped. For example, 1,234.001,234, $42.50$42.5, 0.00%0%.

The change is clean and consistent — all format definitions in both cubejs-client-core and cubejs-schema-compiler are updated in lockstep, tests and snapshots are properly adjusted, and custom-numeric user-provided format strings are correctly left untouched.

What looks good

  • Consistent application: The ~ modifier is applied uniformly across all format families (number, currency, percent, abbr, decimal, accounting, id) in both packages.
  • Test coverage is thorough: Tests cover the key behavioral changes (e.g., $0.00$0, 1,234.001,234, 0.00%0%) and snapshots are updated.
  • User-specified formats preserved: The custom-numeric code path correctly passes user format strings through as-is (line 153 of format.ts), so only defaults change.
  • Dependency compatibility: d3-format ^3.1.0 fully supports the ~ modifier (introduced in v2.0.0).

Items to address

  1. Breaking change for currency display (inline comment posted) — Financial convention strongly favors fixed two-decimal display ($42.50, not $42.5). Consider whether DEFAULT_CURRENCY_FORMAT and the currency_* named formats should keep fixed precision, or document this as an intentional behavior change. This is the main point worth discussing.

  2. Redundant ~ on precision-0 formats (inline comment posted) — Specifiers like ,.0~f, .0~%, .0~s are functionally identical to ,.0f, .0%, .0s since there are no fractional digits to trim. Not a bug, but removing ~ from the _0 variants would be slightly cleaner and avoid implying trimming can happen when it can't. Alternatively, keeping it for consistency is also reasonable — just a nit.

  3. PR description — This is a user-facing behavioral change. A brief description explaining the motivation (e.g., "integer values shouldn't display .00 suffix") and acknowledging it as a breaking change would help reviewers and future changelog readers.

No issues found

  • Security: No security concerns — this is a pure formatting change with no user input handling changes.
  • Performance: No performance impact — ~ is handled natively by d3-format.
  • Architecture: The change is well-scoped to the two relevant packages with no unintended side effects.

Overall this is a clean, well-tested change. The main discussion point is whether currency formats should also trim trailing zeros given financial display conventions.

Comment thread packages/cubejs-client-core/src/format.ts Outdated
Comment thread packages/cubejs-client-core/src/format.ts
Comment thread packages/cubejs-schema-compiler/src/compiler/named-numeric-formats.ts Outdated
@codecov
Copy link
Copy Markdown

codecov Bot commented Apr 30, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 57.99%. Comparing base (f395432) to head (7e73add).
⚠️ Report is 6 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master   #10782   +/-   ##
=======================================
  Coverage   57.99%   57.99%           
=======================================
  Files         215      215           
  Lines       16775    16776    +1     
  Branches     3388     3388           
=======================================
+ Hits         9728     9730    +2     
+ Misses       6554     6553    -1     
  Partials      493      493           
Flag Coverage Δ
cube-backend 57.99% <ø> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

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

@vasilev-alex vasilev-alex merged commit 22694eb into master Apr 30, 2026
73 of 75 checks passed
@vasilev-alex vasilev-alex deleted the change-default-number-format branch April 30, 2026 17:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

client:core Issues relating to the JavaScript client SDK javascript Pull requests that update Javascript code

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants