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 GoString method for Decimal and NullDecimal (#270) #271

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

a69
Copy link

@a69 a69 commented Jan 5, 2022

This PR is for support fmt.GoStringer in #270 issue

@a69 a69 marked this pull request as draft January 8, 2022 06:24
@a69 a69 force-pushed the master branch 2 times, most recently from c97d43b to 47ad6b3 Compare January 8, 2022 09:55
@a69 a69 marked this pull request as ready for review January 8, 2022 09:57
@treuherz
Copy link

I'd find a GoStringer implementation really helpful, for same reasons you describe in #270, but I think it would be more helpful for the method to print fmt.Sprintf("decimal.New(%d, %d)", d.value, d.exp) instead of RequireFromString. This has the advantage of using the most "natural" constructor in the package, and also will actually get you a string that lets you construct a Decimal with the same behaviour.

For example, under this implementation, you could have

d1 := decimal.New(1, 2)
d2 := decimal.New(100, 0)

println(d1.GoString()) // decimal.RequireFromString("100")
println(d2.GoString()) // decimal.RequireFromString("100")
println(d1.Exponent()) // 2
println(d2.Exponent()) // 0

I realise decimal.New(1, 2) is a little bit harder to read an intuitive value from than decimal.RequireFromString("100") but I think it's worth it for more consistent behaviour.

@treuherz
Copy link

Another possible wrinkle here: the user may not have imported the package as decimal. I think the options here are either to leave it, as it'll probably be fine in 99.99% of cases, or remove the decimal. from the method, which could be more confusing but at least won't be contradictory. The fmt package doesn't give explicit guidance here, although their example doesn't include the name.

@a69
Copy link
Author

a69 commented Jan 17, 2022

@treuherz
decimal.New works if d.value always fits in int64.
I think as it says in code comment decimal.go#L5:
// The best way to create a new Decimal is to use decimal.NewFromString

To fix this issue we could do something like:
s := fmt.Sprintf(`decimal.RequireFromString("%s")`, d.string(false)) if d.exp > 0 { s = s + fmt.Sprintf(".Round(%d)", -d.exp) } return s

@mwoss
Copy link
Member

mwoss commented Jan 23, 2022

I've already responded in the linked issue. I think we might try initialization via NewBigInt to not to lose precision.

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.

3 participants