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 BigRational#format #14525

Merged

Conversation

meatball133
Copy link
Contributor

resolves #14062

This is a small change, but it fixes the issue and is perhaps not the most efficient solution; the main goal was to not affect the current code base too much.
In the issues, there are 2 approaches mentioned; I went ahead with this one since if a user wants the result as a float, they can still do that by converting the BigRational to a float.

But I want to present this PR more as a proposal for a solution than as a final solution (thereby, no tests are written/updated). If there is consensus to take the other approach, is that also an option.

@meatball133
Copy link
Contributor Author

Update: I forgot that the big library can't be imported, so the type of the number can't be checked. I implemented an alternative solution, but might that not be that safe?

@HertzDevil
Copy link
Contributor

You could override just this overload in src/big/big_rational.cr itself, that would avoid a rather fragile check for /.

src/big/big_rational.cr Outdated Show resolved Hide resolved
src/big/big_rational.cr Outdated Show resolved Hide resolved
src/big/big_rational.cr Outdated Show resolved Hide resolved
@straight-shoota straight-shoota changed the title Add specefic format logic for BigRational Add specific format logic for BigRational Apr 23, 2024
@straight-shoota straight-shoota changed the title Add specific format logic for BigRational Fix BigRational#format Apr 23, 2024
@straight-shoota straight-shoota modified the milestones: 2.0.0, 1.13.0 May 16, 2024
@straight-shoota straight-shoota merged commit b563cba into crystal-lang:master May 21, 2024
59 of 60 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

BigRational#format is incorrect
5 participants