Implement Digit separator #290
Conversation
|
Thanks for PR! Will review in upcoming days. |
formatter.go
Outdated
| if intg := extractInt(value); intg != nil { | ||
| return f.addDigitGrouping(strconv.Itoa(*intg)) | ||
| } |
There was a problem hiding this comment.
Here we will catch only int, but we should handle other integers types too: uint, int8, int16, etc.
The easiest way to do it is:
- check if isNumber returns true; it means that it's an integer, because floats are already handled above
- if yes, format it using fmt.Sprintf with %v - this should be equivalent to how we format integers below
- apply digit groupping to that string, and return it
formatter.go
Outdated
| @@ -472,6 +497,68 @@ func (f *DefaultFormatter) formatFloatValue(value float64, bits int) string { | |||
| } | |||
| } | |||
|
|
|||
| func (f *DefaultFormatter) addDigitGrouping(numStr string) string { | |||
There was a problem hiding this comment.
I suggest to rename this func to reformatNumber, and groupDigitsInString to insertDigitSeparator.
The first name suggests that we should call it when we're formatting any number, float or int. The caller doesn't bother what exactly it will do.
The second name correlates with the name of the DigitSeparator option.
formatter.go
Outdated
| numStr = numStr[1:] | ||
| } | ||
|
|
||
| parts := strings.SplitN(numStr, ".", 2) |
There was a problem hiding this comment.
To make this code easier to follow, I suggest to extract a function, say, parseNumber, that returns all parts of the number: integer, fractional, and exponent.
Here we can call it, and apply groupping to integer and fractional parts.
Then we can compose number from parts.
formatter.go
Outdated
| parts = strings.SplitN(parts[1], "e", 2) | ||
| // using Reverse(parts[0]) because digit grouping in | ||
| // fractional part is required from Most Significant Bit | ||
| fracPart = "." + Reverse(f.groupDigitsInString(Reverse(parts[0]))) |
There was a problem hiding this comment.
Instead of using Reverse func, I suggest to pass direction flag (-1 or +1) to groupDigitsInString. I think the code will be easier to follow on the caller side
formatter_test.go
Outdated
| @@ -587,6 +587,120 @@ func TestFormatter_FloatFormat(t *testing.T) { | |||
| } | |||
| } | |||
|
|
|||
| func TestFormatter_DigitSeparator(t *testing.T) { | |||
There was a problem hiding this comment.
We also need to cover a few more cases:
- big integer part, small decimal part, non scientific
- big integer part, small decimal part, scientific
- big integer part, big decimal part, non scientific
- big integer part, big decimal part, scientific
By small decimal part, I mean 1 to 3 digits, by big - more than 3 digits.
formatter_test.go
Outdated
| } | ||
| } | ||
|
|
||
| func TestFormatter_DigitSeparatorType(t *testing.T) { |
There was a problem hiding this comment.
We also need a separate test that check how separator is applied to different fliatand integer types (float32, float64, int, uint)
|
☔ The latest upstream change (presumably these) made this pull request unmergeable. Please resolve the merge conflicts. |
default digit separator and minor bug fix
|
I've rebased this branch on master and pushed follow-up commit: 7e55c50 The commit addresses issues I mentioned above. I also switched from manual parsing to regex. Thanks for PR! |
fixes #274