Skip to content
This repository has been archived by the owner on Aug 26, 2022. It is now read-only.

Removing float arithmetic from amount.go #487

Merged
merged 1 commit into from
Jun 3, 2020
Merged

Removing float arithmetic from amount.go #487

merged 1 commit into from
Jun 3, 2020

Conversation

dhwaneetbhatt
Copy link
Contributor

Using float arithmetic with smaller numbers maybe okay because their precision is quite high and we use only 2 decimal points. But with larger numbers, the decimal precision is lost and we don't get back what we intend.

The following test before the code fix used to fail:

// very large number
amt, err = NewAmount("USD", "10000000000000000.20")
if err != nil {
	t.Error(err)
}
if v := amt.String(); v != "USD 10000000000000000.20" {
	t.Errorf("got %q", v)
}

amt.String() used to return 10000000000000000.00

@adamdecaf adamdecaf self-requested a review May 22, 2020 17:58
@adamdecaf
Copy link
Member

Thanks for this change! I cleaned up the code so our linters are happy.

@codecov-commenter
Copy link

codecov-commenter commented May 29, 2020

Codecov Report

Merging #487 into master will increase coverage by 0.35%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #487      +/-   ##
==========================================
+ Coverage   55.73%   56.08%   +0.35%     
==========================================
  Files          71       71              
  Lines        2677     2687      +10     
==========================================
+ Hits         1492     1507      +15     
+ Misses        929      927       -2     
+ Partials      256      253       -3     
Impacted Files Coverage Δ
pkg/model/amount.go 89.61% <100.00%> (+1.55%) ⬆️
pkg/upload/sftp.go 69.10% <0.00%> (+2.61%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 710d093...93550ef. Read the comment docs.

@adamdecaf adamdecaf merged commit 2178b80 into moov-io:master Jun 3, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants