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.

Sign up for GitHub

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

Jump to bottom

Fix BigDecimal#to_big_i regression #8790

Merged
merged 1 commit into from
Feb 18, 2020
Merged

Fix BigDecimal#to_big_i regression #8790

merged 1 commit into from
Feb 18, 2020

Conversation

Sija
Copy link
Contributor

@Sija Sija commented Feb 12, 2020

Fixes #8789

@Sija
Copy link
Contributor Author

Sija commented Feb 12, 2020

Since it's a pretty major regression it would be gr8 having this merged into 0.33.0.

@Sija Sija requested a review from bcardiff February 12, 2020 15:55
Sija
Sija commented Feb 12, 2020
View reviewed changes
spec/std/big/big_decimal_spec.cr

bd1.to_u.should eq 0
bd2.to_u.should eq 0
bd3.to_u.should eq 123
bd4.to_u.should eq 123
bd5.to_u.should eq 123
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@bcardiff btw, why do Big*#to_u methods for values < 0 are returning them as absolute instead of raising?

Copy link
Member

Choose a reason for hiding this comment

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

That seems unexpected...

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 is the effect of time, when Int#to_u used to be more like a cast, and BigDecimal#to_big_u simple uses an .abs as an approximation. Nobody submit any opinions regarding that in the original PR #4876 .

It should raise OverflowError for negative inputs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Was thinking the same, although I'd leave it for another PR.

Copy link
Member

Choose a reason for hiding this comment

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

For further consideration: All these specs don't really test what you would expect. Equality between different number types already works without explicit conversation. So these specs should also validate the return type.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@straight-shoota Could we leave it for another PR? I don't think we should do much more here.

Copy link
Member

Choose a reason for hiding this comment

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

Absolutely

Copy link
Contributor Author

@Sija Sija Feb 13, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

btw, BigInt#to_u / BigFloat#to_u has the same issue:

require "big"

-123.to_big_i.to_u     # => 123
-123.123.to_big_f.to_u # => 123

@Sija Sija mentioned this pull request Feb 12, 2020
Closed
@straight-shoota straight-shoota mentioned this pull request Feb 12, 2020
Closed
straight-shoota
straight-shoota reviewed Feb 12, 2020
View reviewed changes
Copy link
Member

@straight-shoota straight-shoota left a comment

Choose a reason for hiding this comment

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

The fix only works when all decimal places are zero.

This is still incorrect:
"-1.1".to_big_d.to_big_i # => -10

src/big/big_decimal.cr Outdated Show resolved Hide resolved
@Sija Sija force-pushed the fix-8789 branch 2 times, most recently from f5d0812 to c96498f Compare February 12, 2020 17:22
@Sija
Copy link
Contributor Author

Sija commented Feb 12, 2020

@straight-shoota Force-pushed amended commit with the fix.

@straight-shoota
Copy link
Member

Is it possible to (also) have a spec for #ceil which tests the changed behaviour?

@Sija
Copy link
Contributor Author

Sija commented Feb 12, 2020

Is it possible to (also) have a spec for #ceil which tests the changed behaviour?

Done.

@Sija
Copy link
Contributor Author

Sija commented Feb 13, 2020

Is this GTG?

@Sija Sija mentioned this pull request Feb 13, 2020
Closed
@Sija
Copy link
Contributor Author

Sija commented Feb 13, 2020

Merge time? :)

@bcardiff bcardiff added this to the 0.34.0 milestone Feb 14, 2020
@Sija Sija removed the request for review from bcardiff February 16, 2020 17:37
@Sija
Copy link
Contributor Author

Sija commented Feb 18, 2020

Yo! Anything else to do here? 🕦

Your PR-review process guys is a real mystery to me. You let tested, approved and tagged PRs lay idly with no explanation about the next steps or anything else that's suppose to come next... Could you be please more transparent in your actions, so it's less of a guessing-game and more of a understandable process - at least to the outsiders, who do not share your private communication channels or whatever you use to discuss things like that - if you do...

@straight-shoota
Copy link
Member

Sadly, there's really no process in place.
I suppose merging this one was simply delayed to not interfere with the release of 0.33.0. Otherwise @RX14 or @bcardiff probably would've directly merged it.
Sometimes merging approved PRs just gets lost and forgotten. It can be really hard to keep on top of all the things going on. If there's a PR that looks like it could be merged, please just leave a message to bump it.

@straight-shoota straight-shoota merged commit 91fdab2 into crystal-lang:master Feb 18, 2020
@straight-shoota
Copy link
Member

Thanks @Sija and sorry for the delay!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bcardiff bcardiff bcardiff left review comments

@straight-shoota straight-shoota straight-shoota approved these changes

@RX14 RX14 RX14 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
0.34.0
Development

Successfully merging this pull request may close these issues.

Regression in BigDecimal#to_big_i
4 participants
@Sija @straight-shoota @bcardiff @RX14