Add float64 arithmetic #181
Conversation
There was a problem hiding this comment.
Thanks for the PR. I have two questions and a change for the PR.
- Can you please add documentation on these new functions.
- Do you have a use case for adding these functions?
- Why is the decimal conversion happening in the middle? What is the case for this compared to strictly using the way Go handles float64s?
|
No problem, happy to help. I'll take care of 1 shortly. As for 2, these provide a way to do float64 arithmetic, since the existing functions support int64's only. I originally thought about extending those functions to take in interface{}'s and figure out what to do, but I think that has drawbacks. For one, you would need an interface{} return value, which could break existing templates for users of the package. It would also often require casting of the return value. For this and similar concerns, I thought it made sense to be explicit and add new functions specifically for float64's. On 3, it's easy to run into precision issues when doing math with float64's. It may not matter all the time, but if you're relying on these functions for use in currency etc. you need the answer to be precise. shopspring/decimal has good examples of this here: https://github.com/shopspring/decimal#why-dont-you-just-use-float64. In our projects (which are business oriented), whenever we have APIs that manipulate float64s, we always do the math using decimal numbers to avoid these problems. You could also instead have a separate set of functions that take decimals, but this would add more functions and require users to adopt that library. By keeping the conversion behind the scenes you get precise results with standard float64's. |
|
Also, if there's a better way to do the math, I'm all for that. That package is just the option that I'm aware of and have used in the past. |
andrewmostello
force-pushed
the
master
branch
from
a576dc7
to
825e319
Compare
go.mod
Outdated
| @@ -8,6 +8,7 @@ require ( | |||
| github.com/google/uuid v1.1.1 | |||
| github.com/huandu/xstrings v1.2.0 | |||
| github.com/imdario/mergo v0.3.7 | |||
| github.com/shopspring/decimal v0.0.0-20190905144223-a36b5d85f337 | |||
There was a problem hiding this comment.
I was just looking at this package in depth and I have some concerns as it appears to be mostly unmaintained. For example:
- There are only two commits this calendar year
- Numerous bug fixes, improvements, and the move to modules are all sitting as PRs without responses
- There are bugs listed in the issue queue. I wonder if our users will run into these
The company that maintained this package was bought and the service that was using this appears to have been shut down. It looks to have issues while being unmaintained which gives me reason to pause.
I like what it does to make working with floats more practical.
There was a problem hiding this comment.
Do any of the bugs impact this usage?
andrewmostello
force-pushed
the
master
branch
from
825e319
to
b45b459
Compare
|
Thanks for looking into it. There's an issue open on the package concerning its maintenance, and it looks like it has been handed off to new maintainers if that helps resolve the matter: shopspring/decimal#135 |
|
Hey, any plans to merge this PR? |
|
A few quick updates on this PR:
I'm still hoping to get this (or similar functionality) into sprig. Please let me know if there's anything else I can do here. Thanks! |
|
@andrewmostello Thanks for continuing to work on this. |
andrewmostello
force-pushed
the
master
branch
from
059a64a
to
77c017e
Compare
andrewmostello
force-pushed
the
master
branch
from
77c017e
to
8c1c917
Compare
|
No problem! I just rebased again to resolve the conflict. |
|
Question: @mattfarina @technosophos When this feature will be released? Because I am anxious to create a PR in the Helm project to upgrade this lib! |
Add float64 arithmetic functions. For clarity, these functions all have the "f" suffix. To avoid precision errors, all floats are converted to a decimal representation value using shopspring/decimal before calculating.