Fix toFormat grouping by secondaryGroupSize when groupSize is 0 - #411
Fix toFormat grouping by secondaryGroupSize when groupSize is 0#411chatman-media wants to merge 1 commit into
Conversation
groupSize: 0 is supposed to turn grouping off entirely (per MikeMcl#407), but if secondaryGroupSize was also set the two get swapped internally and the check that's meant to disable grouping ends up looking at the secondary value instead. So e.g. { groupSize: 0, secondaryGroupSize: 2 } still grouped '123456789' into '1,23,45,67,89' instead of leaving it alone. Added tests covering this alongside the existing MikeMcl#407 cases.
|
Well, nowhere is it stated that "groupSize: 0 is supposed to disable grouping entirely", so this is not a "fix" but a suggestion to alter the current behaviour, which is how this PR should have been framed. The behaviour has always been that Having said that, it may be a worthwhile change and I will look at it closely presently. |
lbesecker195
left a comment
There was a problem hiding this comment.
Verified in toFormat: with groupSize: 0 and a secondaryGroupSize, the unconditional swap turned g1 into the secondary size and grouping ran anyway, undoing the #407 fix. Gating the swap on g1 > 0 leaves g1 = 0, so the g1 > 0 && len > 0 block is skipped and no grouping happens. Tests cover it. LGTM.
groupSize: 0 is supposed to disable grouping entirely (this was just fixed for the plain case in #407), but it doesn't work if
secondaryGroupSizeis also set. The two values get swapped internally for the Indian-style grouping logic, and thegroupSize > 0check that's meant to short-circuit grouping ends up looking at the secondary value instead.Fixed by only doing the swap when the primary groupSize is actually greater than 0. Added tests next to the existing #407 cases, covering this combination.