Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Appearance settings

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

Morty Proxy This is a proxified and sanitized view of the page, visit original site.