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
base: master
Are you sure you want to change the base?
Fix BigRational#format
#14525
Conversation
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? |
You could override just this overload in |
src/big/big_rational.cr
Outdated
self.denominator.format(io, separator, delimiter, decimal_places, group: group, only_significant: only_significant) | ||
io << '/' | ||
self.numerator.format(io, separator, delimiter, decimal_places, group: group, only_significant: only_significant) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggestion: you can drop self
receiver:
self.denominator.format(io, separator, delimiter, decimal_places, group: group, only_significant: only_significant) | |
io << '/' | |
self.numerator.format(io, separator, delimiter, decimal_places, group: group, only_significant: only_significant) | |
denominator.format(io, separator, delimiter, decimal_places, group: group, only_significant: only_significant) | |
io << '/' | |
numerator.format(io, separator, delimiter, decimal_places, group: group, only_significant: only_significant) |
BigRational#format
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.