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

(Issue #10) num_rational::Ratio: Add the ability to format as a decimal #37

Closed
wants to merge 5 commits into from

Conversation

Hazardius
Copy link

Added simple formatting of Ratio as a decimal approximation to the fmt if it's given a Formatter with precision.

After is compiled and passed some new tests for formatting with precision in Formatter I've run the file through the cargo fmt before the commit.

As it's my first contribution in rust - I'm open for suggestions of improvements.

…approximation to the fmt if it's given a Formatter with precision.
@cuviper
Copy link
Member

cuviper commented Jul 28, 2018

Hi, thanks for the PR!

Some of your new where constraints are redundant -- Integer in particular will imply most of the rest. But sadly, adding constraints to this impl is a breaking change, so we can't do it exactly like this right now.

Maybe we can do it as a new method instead? Something like fn as_decimal(&self, precision: usize) -> String, although that implies std-only for String. Another option is to take a generic W: fmt::Write argument, but that's not as easy for the user. Yet another way is to return a formattable wrapper type, having that implement a decimal-style Display. Lots of possibilities...

I also worry that multiplying 10^n is going to be really prone to overflow, but we can think more deeply about that once the API is sorted out.

@Hazardius
Copy link
Author

Thank you very much for your feedback.
I'll try to sit down and rethink it next week after Wednesday.

…he separate method 'as_decimal' returning String.
@Hazardius
Copy link
Author

I was unable to sit with that issue before now, but here's silly separation of my code to the new as_decimal method.
Created a new trait for it and tried to remove as much of traits associated with T as I can.

src/lib.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
+ FromPrimitive
+ Integer
+ Pow<usize, Output = T>
+ Signed
Copy link
Member

Choose a reason for hiding this comment

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

Why do you need Signed?

Copy link
Author

Choose a reason for hiding this comment

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

I'm pretty sure that I've added it to be able to use fn abs(&self).
Couldn't find a way to drop it now.

src/lib.rs Outdated
+ Integer
+ Pow<usize, Output = T>
+ Signed
+ std::string::ToString,
Copy link
Member

Choose a reason for hiding this comment

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

This is implied by Display.
(via impl<T> ToString for T where T: Display)

Copy link
Author

Choose a reason for hiding this comment

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

Got rid of it from the type traits, but needed to add use std::string::ToString; elsewhere to make it compile.

src/lib.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
@Hazardius
Copy link
Author

Closing this PR as it's pretty much stagnant at this point.

@Hazardius Hazardius closed this Apr 8, 2019
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.

2 participants