Skip to content

fix(chart) :: drop unreachable branch - #1506

Open
81reap wants to merge 1 commit into
mainfrom
81reap/ts-3
Open

81reap wants to merge 1 commit into
mainfrom
81reap/ts-3

Conversation

@81reap

@81reap 81reap commented Sep 29, 2026

Copy link
Copy Markdown
Collaborator

The check > 10 and NaN (a 3 char string) cannot both hold at the same time.

Removes the branch and sets maximumFractionDigits: 2.


Stack created with GitHub Stacks CLI • Give Feedback 💬

@81reap
81reap added this pull request to stack #1512 September 29, 2026 03:56

@lovasoa lovasoa left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

the checks are not equivalent are they? do you have examples of differences in concrete charts and why you think the new version is better? If we change digit rendering in charts, we must explain the impact to users in CHANGELOG

Base automatically changed from 81reap/ts-2 to main September 29, 2026 05:39
The check `> 10` and NaN (a 3 char string) cannot both hold at the same time.

Removes the branch and sets `maximumFractionDigits: 2`.
@81reap

81reap commented Sep 29, 2026

Copy link
Copy Markdown
Collaborator Author

I'm not quite sure I follow. the branch is impossible to reach in most locales AFAICT so it was falling back to the Locale default length (which is 3 in "en"). If you run this snippet in your browser

const before = (value, locale) => {
  const str_val = value.toLocaleString(locale);
  if (str_val.length > 10 && Number.isNaN(value)) return value.toFixed(2);
  return str_val;
};

const after = (value, locale) =>
  value.toLocaleString(locale, { maximumFractionDigits: 2 });

const LOCALES = ["en", "fr", "de", "ja", "ar-EG", "fa-IR", "my", "am"];

// (1) maximumFractionDigits: 2 — drops a digit in every locale.
const digits = {};
for (const locale of LOCALES) {
  const row = {};
  for (const value of [1.2345, 0.005, 1234.5678, 1234567890.12345]) {
    const b = before(value, locale);
    const a = after(value, locale);
    row[value] = b === a ? `${b}  (same)` : `${b} → ${a}`;
  }
  digits[locale] = row;
}
console.table(digits);

// (2) The removed branch — only fires where NaN formats past 10 characters.
console.table(
  LOCALES.map((locale) => ({
    locale,
    toLocaleString: Number.NaN.toLocaleString(locale),
    "branch fires": Number.NaN.toLocaleString(locale).length > 10,
    before: before(Number.NaN, locale),
    after: after(Number.NaN, locale),
    changed: before(Number.NaN, locale) !== after(Number.NaN, locale),
  })),
);

then you will get

Screenshot_2026-09-29_01-57-37

from my interpretation of the code. the goal was to format to 2 decimal places. if you can provide more context about the intent of the code then I'm sure I can adjust the options being sent into toLocaleString to match

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants