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 Format() #281

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

Add Format() #281

wants to merge 1 commit into from

Conversation

mcdee
Copy link

@mcdee mcdee commented Apr 11, 2022

This adds a Format() function to decimal, allowing for setting of a thousands separator (as requested in #267) and decimal separator, as well as the existing functionality for trimming trailing zeros. This allows for easier use in localized environments.

This replaces the prior internal string() function, which could now be removed entirely but that would result in a larger PR so has been left for now.

Worth taking a close look at lines 1032-1034 to ensure they behave as expected, as they alter the flow compared to before this PR.

Copy link
Member

@mwoss mwoss 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 contribution and I deeply apologize for my late review. 🙏

The ability to format the string representation seems like a nice addition to the library. If I remember correctly, this feature has been already requested in the past.
Sadly, there's one big problem. We cannot use the Format as a method name. Go does not support method overloading and there's the fmt.Formatter interface that needs to be implemented on Decimal struct one day. Implementing this interface would allow the comprehensive use of decimals in strings. So I think we should not block it by using this name.

I'm also thinking about whether localized-like Format method should be a part of the Decimal struct. If we would implement fmt.Formatter, users could freely use packages like golang.org/x/text/message to format the output to their needs. They offer more advanced formatting options and probably would fulfill all current and future devs needs. What do you think?

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.

2 participants