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

Add float64 arithmetic #181

Merged
merged 1 commit into from
Dec 7, 2020
Merged

Conversation

andrewmostello
Copy link
Contributor

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.

Copy link
Member

@mattfarina mattfarina left a comment

Choose a reason for hiding this comment

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

Thanks for the PR. I have two questions and a change for the PR.

  1. Can you please add documentation on these new functions.
  2. Do you have a use case for adding these functions?
  3. Why is the decimal conversion happening in the middle? What is the case for this compared to strictly using the way Go handles float64s?

@andrewmostello
Copy link
Contributor Author

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.

@andrewmostello
Copy link
Contributor Author

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.

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
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

Do any of the bugs impact this usage?

@andrewmostello
Copy link
Contributor Author

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

@Moser-ss
Copy link

Hey, any plans to merge this PR?
I am using sprig inside of the Helm template, and I had a case where I need to multiply an int64 to with float64. In this case, I want to increase 1GB with 0.5 (that means cut the value in half), and I can't because the math functions don't accept float64 either return a float64.
What is blocking this PR, and exists something I can help to push this?

@andrewmostello
Copy link
Contributor Author

A few quick updates on this PR:

  • I just rebased my fork again, so the PR is the addition of one commit
  • shopspring/decimal is actively being maintained, and they've been tagging releases since April
  • I've updated the dependency to the latest tag release

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!

@mattfarina
Copy link
Member

@andrewmostello Thanks for continuing to work on this.

Copy link
Member

@technosophos technosophos left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks for seeing this through!

@andrewmostello
Copy link
Contributor Author

No problem! I just rebased again to resolve the conflict.

@mattfarina mattfarina merged commit be8a535 into Masterminds:master Dec 7, 2020
@Moser-ss
Copy link

Moser-ss commented Dec 8, 2020

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!

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.

4 participants