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

Revert remove insignificant digits #159

Merged
merged 2 commits into from
Feb 27, 2020
Merged

Revert remove insignificant digits #159

merged 2 commits into from
Feb 27, 2020

Conversation

njason
Copy link
Member

@njason njason commented Dec 10, 2019

When I went to revert #46, TestDecimal_RoundCash started failing because the RoundCash function (#66) relies on a Decimal with insignificant digits removed. To get around this problem I made a new function to remove insignificant digits

@njason njason requested a review from mwoss December 10, 2019 05:56
@mwoss
Copy link
Member

mwoss commented Jan 2, 2020

Currently, there is a bug in RoundCash method, mentioned in #89. We have to fix this issue before merging this PR.

@njason njason closed this Feb 26, 2020
@njason njason deleted the insig-func branch February 26, 2020 01:02
@njason njason restored the insig-func branch February 26, 2020 01:04
@njason njason reopened this Feb 26, 2020
@njason njason changed the title RemoveInsignificantDigits function Revert remove insignificant digits Feb 26, 2020
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.

It looks good, ready to merge :3

@philippgille
Copy link

philippgille commented Apr 3, 2020

@njason @mwoss

Thanks for maintaining this library 💪 ! A quick question regarding this PR:

Is it correct that when using Decimal in a struct and encoding an object of that struct and later decoding it (for example during persistence and retrieval from a DB) and comparing the two objects in a test with something like require.Equal() (which uses reflection.DeepEqual() under the hood), that after this PR the objects are seen as unequal, whereas before they were seen as equal?

And the correct way to compare them would be to either use String() on both objects or RescalePair() first?

@mwoss
Copy link
Member

mwoss commented Apr 6, 2020

Hi @philippgille. I apologize for the late response, I had a tough weekend.
The scenario you've described should not have happened. Are you using correct struct tags so values can be properly serialized/deserialized? Could you give us a small code snippet to reproduce this issue?

@philippgille
Copy link

For creating a snippet I had another look and the root cause seems to be that the initial decimal is created with Round(6), while the Postgres database column is decimal(x, 8). So after fetching the number from the DB and deserializing it, a decimal with 8 decimal places is created. But the last two digits are 00, because the original decimal just had 6 decimal places. Now before this PR these two trailing zeroes were removed and thus the reflect.DeepEqual() check yielded true, while after this PR the trailing zeroes are not removed and thus the check yields false.

So in the end I'd say this library behaves correctly and as expected (8 decimal places in the DB lead to 8 decimal places in the deserialized field). It's just a breaking or at least noteworthy change.

@mwoss
Copy link
Member

mwoss commented Apr 8, 2020

@philippgille Yes, if those decimals differ by two last zeroes after a comma, they for sure, should not be equal. This was discussed in depth in #107.

@philippgille
Copy link

Does it make sense to add a note to the README or so, because the behavior was different for almost exactly 3 years, so I can imagine other people updating the package and wondering about failing unit tests as well :)

@mwoss
Copy link
Member

mwoss commented Apr 9, 2020

I'm 100% agreeing with you. I'm adding this task to our backlog.
Thanks a lot :))

fairyhunter13 added a commit to fairyhunter13/decimal that referenced this pull request Jul 12, 2020
* revert original remove insignificant change (PR 46), and add the logic back as a function
@daemonfire300
Copy link

I'm 100% agreeing with you. I'm adding this task to our backlog. Thanks a lot :))

I could not find the backlog issue so pardon me for reviving this zombie thread 😄

#277

@tzachshabtay
Copy link

We just got burned by this breaking change after upgrading the package.
We need to know the precision ignoring trailing zeroes (i.e what Exponent used to do), my current workaround is:

func getOldExponent(x decimal.Decimal) int32 {
	return decimal.RequireFromString(x.String()).Exponent()
}

Is there a better way? @mwoss (p.s thank you for this library).

@mwoss
Copy link
Member

mwoss commented Jan 10, 2024

I deeply apologize for my late reply! I was not actively maintaining this library in the past year :<


@daemonfire300 - Sure I will take a look at your PR

@tzachshabtay - I apologize for that. We should've communicated this change more broadly or introduced it in the 2.x release. Right now library does not provide an easy way to calculate precision ignoring trailing zeroes. You can do it your way or count the trailing zeroes and substract it from Exponent:

func getPrecisionWithoutTrailingZeroes(d Decimal) int32 {
	coeff := d.Coefficient().String()
	trailingZeroes := len(coeff) - len(strings.TrimRight(coeff, "0"))
	return min(0, d.Exponent()+int32(trailingZeroes)) // for positive exponents decimal precision is always 0
}

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.

5 participants