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
added alternate forms of '/' and set default Display to '⁄': FRACTION SLASH #101
Conversation
de3daca
to
3e3f636
Compare
fd7bdc6
to
49b7f2c
Compare
Still need to make clear its "reluctant" e.g. wont error if theres bogus after the string. |
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.
Brilliant, thank you for the contribution!
One thing I wanted you to consider is the API structure. I feel like we could decouple the particular unicode display configuration aspects from the GenericFraction surface into its own separate scope owned by a shared UnicodeDisplay
structure. We probably only need one method on GenericFraction to get that display with the relevant configurations. That structure could probably expose more methods with extra configuration elements (e.g. with enums or bitmasks).
@dnsl48 done! it's not the most most beautiful code atm, but it works. Clippy was complaining about manual stripping. |
} else if s.starts_with('⅓') { | ||
Ok(GenericFraction::Rational( | ||
sign, | ||
Ratio::new_raw(1.into(), 3.into()), |
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.
because of this
let sign = if input.starts_with('-') { | ||
s = &input.strip_prefix('-').unwrap(); | ||
Sign::Minus | ||
} else if input.starts_with('+') { | ||
s = &input.strip_prefix('+').unwrap(); | ||
Sign::Plus |
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.
these double checks are here, because Clippy complained about manual stripping. Hopefully, the compiler gets rid of it
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.
Overall it looks really nice!
I've left a couple of comments where we could polish it a bit more.
"11⁄2", // uses INVISIBLE_TIMES | ||
]; | ||
for s in test_vec { | ||
println!("{}", s); |
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.
redundant output?
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.
Not entirely, it allows you to see where the test fails, because the last one will be printed. The assert_eq!
won't give helpful output.
feefadd
to
feefadd
Compare
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.
Thank you for the contribution, great work!
outlined in #100 Which character to use for the slash in output may be discussed, but I chose "FRACTION SLASH" since it's "fraction" crate.