Skip to content

fix(drafting): keep the group separator off the minus sign - #175

Open
rajanpanth wants to merge 1 commit into
accordproject:mainfrom
rajanpanth:fix/negative-number-grouping
Open

fix(drafting): keep the group separator off the minus sign#175
rajanpanth wants to merge 1 commit into
accordproject:mainfrom
rajanpanth:fix/negative-number-grouping

Conversation

@rajanpanth

Copy link
Copy Markdown

Problem

Formatting a negative number with a grouped format puts the group separator immediately after the minus sign:

getDrafter('Integer')(-123, '0,0')          // '-,123'      expected '-123'
getDrafter('Integer')(-999, '0,0')          // '-,999'      expected '-999'
getDrafter('Integer')(-123456, '0,0')       // '-,123,456'  expected '-123,456'
getDrafter('Double')(-123456.78, '0,0.00')  // '-,123,456.78'
getDrafter('Long')(-123456, '0,0')          // '-,123,456'

Cause

draftIntegerFormat and draftDoubleFormat group the integer part by repeatedly peeling off its last three characters:

let i = vs.substring(0, vs.length);
while (i.length > 3) {
    res = sep1 + i.substring(i.length - 3) + res;
    i = i.substring(0, i.length - 3);
}
return i + res;

vs comes from value.toFixed(...), so for a negative number it starts with -. The loop treats that sign as if it were a digit. Once the digits have been consumed, i is left holding just "-", and since res already begins with a separator the result is -,123.

This only shows up when the digit count is a multiple of three. With -1234 the sign plus four digits leaves a two character remainder, so -1,234 comes out correctly, which is why the common cases look fine.

Fix

Separate the sign before grouping and put it back afterwards:

+const sign = i.startsWith('-') ? '-' : '';
+i = i.substring(sign.length);
 while (i.length > 3) {
     res = sep1 + i.substring(i.length - 3) + res;
     i = i.substring(0, i.length - 3);
 }
-return i + res;
+return sign + i + res;

Applied to both Double/format.ts and Integer/format.ts. Long imports draftIntegerFormat, so it is covered by the same change.

After the fix:

getDrafter('Integer')(-123, '0,0')          // '-123'
getDrafter('Integer')(-123456, '0,0')       // '-123,456'
getDrafter('Double')(-123456.78, '0,0.00')  // '-123,456.78'
getDrafter('Double')(-1234.5, '0,0.00')     // '-1,234.50'

Testing

Added cases to test/DraftFormat.test.ts covering negative Integer, Long and Double values at the digit counts that trigger the bug, plus a check that positive grouping is unchanged.

Reverting the two source files while keeping the new tests fails 3 of them, so they cover the change rather than restating current behaviour.

On the full suite, test/JavaScriptEvaluator.test.ts, test/GenerateOptions.test.ts and test/TemplateMarkInterpreter.test.ts fail for me, but they fail identically on a clean checkout of main, so they look unrelated to this change. Excluding those, the suite goes from 92 to 96 passing with the added tests and no new failures.

draftIntegerFormat and draftDoubleFormat group the integer part by
repeatedly taking its last three characters while more than three
remain. The minus sign is part of that string, so it counted as a digit
and a separator was inserted directly after it whenever the number of
digits was a multiple of three:

    getDrafter('Integer')(-123, '0,0')        // '-,123'
    getDrafter('Integer')(-123456, '0,0')     // '-,123,456'
    getDrafter('Double')(-123456.78, '0,0.00')// '-,123,456.78'

Numbers such as -1234 were unaffected, because the sign plus four
digits left a two character remainder, which hid the bug for the
lengths people usually try.

The sign is now separated before grouping and prepended afterwards.
Long shares Integer's implementation, so it is fixed too.

Signed-off-by: rajanpanth <rajan.pantha@samriddhicollege.edu.np>
@rajanpanth
rajanpanth requested review from a team and a lite review from Copilot August 23, 2026 15:12

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

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