search

dnsl48 / fraction

[Rust] Lossless fractions and decimals; drop-in float replacement
Apache License 2.0
83 stars 25 forks source link

FromStr trait support + Parse fractions #52

Closed scott-wilson closed 2 years ago

scott-wilson commented 2 years ago

It looks like the fraction library doesn't support parsing fractions in the format of 1/2. I wrote a toy Postgres type (https://github.com/scott-wilson/pg_fraction), and I'd like to move converting the fraction to this crate.

If this is not currently supported, then I suggest adding support for FromStr::from_str. Also, I recommend using nom for parsing the string, since it makes parsing strings fairly easy (see https://github.com/scott-wilson/pg_fraction/blob/e799ca9102a7f34b7a7d3241a5fcfda4a7b2be57/src/lib.rs#L31)

I'll happy set up a PR to add support for parsing fractions, and also to use nom for parsing (including updating GenericFraction::from_decimal_str to the new parser). Let me know if this is something for me to tackle, and I'll start on it this week.

dnsl48 commented 2 years ago

Hi @scott-wilson, thank you for your feedback and for the suggestion! Yes, indeed, we don't have support for "1/2", although that feels rather trivial to split in two integers and, perhaps, might even use Rust own built-in string to integer parsing capabilities. Nonetheless, it might be nice to have a shortcut for that in the library API, especially that it is the format we produce via Display. If you have an interest in providing a PR for that, that would be appreciated!

using nom for parsing the string

Yes, nom is a powerful tool. We only have rather trivial parsing in this crate, so I don't feel a strong need to use it, but if you provided the implementation in your PRs, that would be nice (ideally a separate PR with refactoring and a separate one for parsing "1/2"). For the refactored code I would appreciate having a benchmark as well, to make sure we don't have a performance regression.

scott-wilson commented 2 years ago

Sounds good. I'll take a crack at it and see how it benchmarks.

dnsl48 commented 2 years ago

Thank you for the contribution! Released in 0.10.0.

P.S. Regarding refactoring that with nom it feels like it might not be worth pulling nom to only cover our existing parsing needs. Having nom as a hard dependency could cause version conflicts with external projects' own nom. Feels like maybe the benefits won't outweigh the downsides in that case. Closing this issue for now, but please reopen if you'd like to keep that discussion going :)

scott-wilson commented 2 years ago

Yeah, I feel like you're right. Thanks for the merge!