fix: grouping separator for float and decimal#20268
Conversation
kosiew
left a comment
kosiew
left a comment
There was a problem hiding this comment.
Thanks for working on this.
| number = insert_thousands_separator(&number); | ||
| } |
There was a problem hiding this comment.
Would this be applied unconditionally for decimal formatting, including scientific / compact-scientific output?
There was a problem hiding this comment.
Yeah it does, will move it to only apply on non scientific output
There was a problem hiding this comment.
@Druva-D
Thanks for the iteration.
The changes look solid and the new behavior is well thought through. I left a few small suggestions that could help reduce duplication and tighten consistency, especially if this area evolves further.
| StringArray | ||
| ); | ||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
I think one test case that was explicitly called out earlier is still missing.
The combination %(,10.2f (parentheses for negatives + grouping + width) was mentioned alongside %0,10.2f. The %0 case is covered now, but I could not find a test for the %( case.
From a quick read, format_float seems to already handle negative_in_parentheses correctly, including placing commas in the integer part. So this is probably working fine, but it would be great to have a test to confirm the behavior.
Something like:
// %(,12.2f with -1234.5
// Expect something like "(1,234.50)" with padding depending on width
// Worth verifying against Java: String.format("%(,12.2f", -1234.5)
Also worth noting that format_decimal currently ignores negative_in_parentheses and always uses a minus sign. That looks like pre-existing behavior, but adding the decimal version of this test would help document that gap more clearly.
There was a problem hiding this comment.
Yep, looks like its not being handled in format_decimal, added a test and a TODO to document, will implement it if I get time.
2 participants
Which issue does this PR close?
What changes are included in this PR?
Created a new function to insert the separator (,) into the numbers if the flag is enabled
Are these changes tested?
Tests passed locally
Added unit tests to verify functionality
Are there any user-facing changes?
No
No changes to public api
Additional Info
Claude was used to assist in identifying the source of the issue.