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

Fix #295 | String Trim Trailing Zeros bool variable #296

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

vasilbekk
Copy link

@vasilbekk vasilbekk commented Oct 1, 2022

Fix #295

@vasilbekk vasilbekk changed the title Fix #295 | String trim trailing zeros global var Fix #295 | String Trim Trailing Zeros bool variable Oct 1, 2022
@mcdee
Copy link

mcdee commented Oct 8, 2022

Also see #281 which provides this plus additional functionality.

@davseby
Copy link
Contributor

davseby commented Oct 12, 2022

Also see #281 which provides this plus additional functionality.

This is different as by globally setting the trim trailing zero bool value you don't need to handle the JSON MarshalText or MarshalJSON methods for structures. The Format method would still require you to do so.

@vasilbekk
Copy link
Author

vasilbekk commented Oct 26, 2022

Also see #281 which provides this plus additional functionality.

This is different as by globally setting the trim trailing zero bool value you don't need to handle the JSON MarshalText or MarshalJSON methods for structures. The Format method would still require you to do so.

Absolutely! Thank you for your attention and answer, it's really pleasant.

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.

I apologize for the late review. Seems like a great addition to library, thanks!

// For example, if you have numeric(10,2) values stored in your database,
// and you want your API response to always be given 2 decimal places (even 2.00, 3.00, 17.00 [not 2,3,17]),
// then this variable is a great way out.
var StringTrimTrailingZeros = true
Copy link
Member

Choose a reason for hiding this comment

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

I think we can simply call it TrimTrailingZeros, trimming is an action widely used on strings in various languages. Or do you think it could be a bit misleading as NewFromString or RequiredFromString would not perform trimming?

Copy link
Member

Choose a reason for hiding this comment

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

I also wonder whether we should introduce another config var for specifying NewFromString behaviour. The current implementation does not trim zeroes during the initialization from string, but we saw in the past that some developers would like to see such an option.
Unfortunately, we cannot use this newly introduced variable for both of these cases as we would break backward compatibility.

@@ -3339,3 +3339,48 @@ func ExampleNewFromFloat() {
//0.123123123123123
//-10000000000000
}

func TestDecimal_String(t *testing.T) {
Copy link
Member

Choose a reason for hiding this comment

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

Could we separate test cases for TrimTrailingZeros=true and TrimTrailingZeros=false? Thanks to that we could easily catch potential bugs and it would improve overall code maintainability.

Comment on lines +55 to +60
// StringTrimTrailingZeros should be set to false if you want the decimal stringify without zeros trailing.
// By default, when decimal is output as a string (for example, in JSON), zeros are truncated from it (2.00 -> 2, 3.11 -> 3.11, 13.000 -> 13).
// But this logic can be changed by this variable.
// For example, if you have numeric(10,2) values stored in your database,
// and you want your API response to always be given 2 decimal places (even 2.00, 3.00, 17.00 [not 2,3,17]),
// then this variable is a great way out.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// StringTrimTrailingZeros should be set to false if you want the decimal stringify without zeros trailing.
// By default, when decimal is output as a string (for example, in JSON), zeros are truncated from it (2.00 -> 2, 3.11 -> 3.11, 13.000 -> 13).
// But this logic can be changed by this variable.
// For example, if you have numeric(10,2) values stored in your database,
// and you want your API response to always be given 2 decimal places (even 2.00, 3.00, 17.00 [not 2,3,17]),
// then this variable is a great way out.
// TrimTrailingZeros specifies whether trailing zeroes should be trimmed from a string representation of decimal.
// If set to true, trailing zeroes will be truncated (2.00 -> 2, 3.11 -> 3.11, 13.000 -> 13),
// otherwise trailing zeroes will be preserved (2.00 -> 2.00, 3.11 -> 3.11, 13.000 -> 13.000).
// Setting this value to false can be useful for APIs where exact decimal string representation matters.

Copy link
Member

Choose a reason for hiding this comment

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

What do you think about it? A bit simplified and a bit more explicit version of your suggestion.

@shuqingzai
Copy link

Hope this PR will be completed soon.

@JohnAD
Copy link

JohnAD commented Dec 1, 2024

@mwoss @vasilbekk Is there upcoming progress on this? It is a road-blocker for me. A library I'm writing reads a JSON doc with Decimal marshaling. So if it reads

{ "foo": 12.300 }

and I make no changes to the doc and write it back out again, is it writing

{ "foo": 12.3 }

For my library, which does git-tracking of JSON, this is a serious problem.

If this has stalled, let me know. I'm happy to clone this branch and make an appended PR with the changes you have suggested.

While I'm at it I could also add a Match method that both compares the "scalar" value (like Equal does) as well as the exponent. So, (psuedo-code) "12.3".Equals("12.30") but !"12.3".Match("12.30").

In the meantime I'm using the greatcloak fork. But that one has a dependency on the entire MongoDB driver. That is too much overhead for my project.

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.

String Trim Trailing Zeros bool variable
7 participants