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

allow more uses of the zero value #16

Closed
josharian opened this issue Jun 9, 2022 · 3 comments
Closed

allow more uses of the zero value #16

josharian opened this issue Jun 9, 2022 · 3 comments

Comments

@josharian
Copy link
Contributor

josharian commented Jun 9, 2022

Hi! Thanks a bunch for this very useful package.

Consider summing a slice of amounts. The natural code to write is something like:

func Sum(amounts []currency.Amount) (currency.Amount, error) {
	var sum currency.Amount
	for _, x := range amounts {
		var err error
		sum, err = sum.Add(x)
		if err != nil {
			return currency.Amount{}, err
		}
	}
	return sum, nil
}

However, this code doesn't work, because sum gets initialized to the zero value, which has an empty currency code. Playground: https://go.dev/play/p/SzMyDcxcos7

I propose that Add and Sub the zero value (0 number, empty currency code) as special, and ignore currency mismatches in this case, adopting the non-empty currency code.

I also hit something similar in currency.Amount.Scan. I want to be able to store the zero value in the database, as a "not yet initialized, unknown currency" value. But loading such a value from the database using currency.Amount.Scan fails, because the empty currency code is not valid. I similarly propose that Scan have a special case for number = 0 and currency code consisting of all spaces ("" or " "), and allow it to be parsed without error.

Thanks again.

@josharian
Copy link
Contributor Author

Oh, and I should say that I'm happy to send a PR, if this change is welcome.

It is a slight behavioral change, but one that turns an error into a non-error, so it shouldn't cause too many backwards compatibility problems.

@bojanz
Copy link
Owner

bojanz commented Jun 14, 2022

Thank you for your feedback, and for validating the approach in advance.

Both suggestions sound reasonable. I suggest doing them in separate PRs (one for Scan, one for the CurrencyMismatch on empty) to simplify merging.

josharian added a commit to josharian/currency that referenced this issue Jun 14, 2022
josharian added a commit to josharian/currency that referenced this issue Jun 14, 2022
josharian added a commit to josharian/currency that referenced this issue Jun 16, 2022
bojanz pushed a commit that referenced this issue Jun 28, 2022
@bojanz
Copy link
Owner

bojanz commented Jun 28, 2022

All done. Thank you, and welcome to the contributor list :)

@bojanz bojanz closed this as completed Jun 28, 2022
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

No branches or pull requests

2 participants