Skip to content

Rational that reduce to ints should print like ints #15329

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

Closed
wants to merge 2 commits into from

Conversation

pfalabella
Copy link
Contributor

When you print rational expressions it currently looks bad to see integers formatted as fractions with a denominator of 1 (even though that's how they are stored internally).
For instance, in http://is.gd/0gRT1T,

println!("{}", Ratio::<int>::from_integer(42)); // currently prints "42/1"
                                                                     // this PR allows it to print as "42"

@pfalabella pfalabella closed this Jul 2, 2014
@pfalabella pfalabella reopened this Jul 2, 2014

num.and_then(|n:T| den.clone().and_then(|d: T| {
Some(Ratio::new(n.clone(), d))
}))
Copy link
Member

Choose a reason for hiding this comment

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

You should be able to avoid the clone calls here by using explicit pattern matching rather than helper methods:

match (num, den) {
    (Some(n), Some(d)) => Some(Ratio::new(n, d)),
    _ => None
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

tried it and you're right. Plus it looks much more readable too... Thanks!

@alexcrichton
Copy link
Member

Looks good to me, thanks! Could you squash these commits together?

@pfalabella pfalabella closed this Jul 2, 2014
@pfalabella
Copy link
Contributor Author

@alexcrichton sorry, I messed up the rebasing. I'll push a fresh commit referencing this one.

@pfalabella pfalabella deleted the rational-show branch July 3, 2014 11:34
bors added a commit that referenced this pull request Jul 3, 2014
Tried squashing commits for #15329, failed spectacularly.
This is a fresh PR for the same change.

@alexcrichton?
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