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

uint64 to float64 refactoring #173

Closed
wants to merge 1 commit into from
Closed

Conversation

divyashie
Copy link

- Summary
Fixed issue #153.
Motivation: This fix will help increase precision in the calculations on the gocommerce platform.

- Test plan
Make test
In observation of the test cases expected values and the exactness of the new floating point dollar amounts, I believe it is appropriate to change Assert.Equal to Assert.InDelta. The latter assert, which takes in a Tolerance value, allows for comparing the expected value and actual value under a certain precision.

- Description for the changelog
uint64 to float64 refactoring

image

Copy link
Contributor

@mraerino mraerino left a comment

Choose a reason for hiding this comment

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

Thank you for looking into this.

I have to strongly disagree with the decision to turn every amount into a float.
Floats have several issues and because of rounding anomalies are not a good fit for calculating monetary amounts.

Some articles around this:
https://husobee.github.io/money/float/2016/09/23/never-use-floats-for-currency.html
https://spin.atomicobject.com/2014/08/14/currency-rounding-errors/

You also used a float on values that really don't make any sense with decimals (e.g. quantity or DiscountItem.Fixed).

We definitely have to find a way to represent decimals of any length in some cases (percentage discounts & taxes), but I see no purpose in turning everything into a float, especially not currency amounts where it might be harmful.

@bcomnes
Copy link
Contributor

bcomnes commented Mar 1, 2019

@divyashie Thank you very much for the PR, interest and time! I'm going to close this approach for now for the reasons @mraerino mentioned, but if you are still interested on the problem, lets discuss ideas in #153

@bcomnes bcomnes closed this Mar 1, 2019
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

3 participants