Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: adding in support for commas #183

Closed
wants to merge 1 commit into from

Conversation

ConradKurth
Copy link

We have a situation where we are reading in values from a CSV using gocsv, we would like to have support for stripping out commas from numbers. We believe this is reasonable support since having numbers with commas is a reasonable annotation to numbers.

Would love to hear your thoughts or if my additional of a test is not satisfactory, let me know!

@mwoss
Copy link
Member

mwoss commented Aug 27, 2020

Hi @ConradKurth! Thanks for your contribution. Also, it's nice to hear that you are using decimal lib too.

I was thinking about your solution for a bit and I came to the conclusion that we should not merge it in the state like this.
The reasoning is straight forward. Merging this PR could be a cause of crashing dozens of pipelines that already use decimal library. Even though changes you introduced are fairly small, and at first glance should not be problematic for anyone, they are touching core initialization logic that probably every user of decimal lib is using. Devs that depended on how NewFromString worked could be surprised that method is not failing on values like 61,,,,,1 or even correct one like 5,199.2.
By design minor version bump should not introduce any breaking changes - we know it's not always true, but let's not talk about it much :DD.
In my opinion, the best idea here would be to introduce an optional parameter for a NewFromString method, called thousandSeparator. It would keep backward compatibility and also give users the freedom to configure their own separator. Unfortunately, the Go language does not provide such capability, so it's a no-op, at least for now.

So, I would propose is an introduction of NewFromFormattedString method (or something similar in naming) that would handle various decimal representation like those with thousand separators (12,000.00; 12_000.0), money signs ($5.12, €55.12), etc.
A similar idea was mentioned in PR #145. I kinda deprioritized it as I thought is not a necessary feature, but now I think more people need it than I expected. I will start the implementation this week.
This should be released in version 1.3.0 after I finish unfamous Pow method reimplementation.

In version 2.X both NewFromString and NewFromFormattedString could be merged into one method.

What do you think @ConradKurth?

@mwoss
Copy link
Member

mwoss commented Sep 30, 2020

The requested functionality has been implemented in #184

@mwoss
Copy link
Member

mwoss commented Oct 27, 2020

Due to no response and merged implementation #184 I'm closing this pull request.

@mwoss mwoss closed this Oct 27, 2020
@coryjohnson-flume coryjohnson-flume restored the master branch January 13, 2023 17:10
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.

None yet

2 participants