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

Implement more formatting traits #1

Closed
cuviper opened this issue Dec 19, 2017 · 3 comments · Fixed by #56
Closed

Implement more formatting traits #1

cuviper opened this issue Dec 19, 2017 · 3 comments · Fixed by #56

Comments

@cuviper
Copy link
Member

cuviper commented Dec 19, 2017

LowerExp, UpperExp, Binary, Octal, LowerHex, and UpperHex are all reasonable, where the underlying type already implements them.

cc rust-num/num#259

Phlosioneer added a commit to Phlosioneer/num-rational that referenced this issue Feb 8, 2018
Added impls for the various fmt traits: Octal, LowerHex, UpperHex, Binary, LowerExp, UpperExp
Also added tests for those impls.

Closes rust-num#1
@maxbla
Copy link
Contributor

maxbla commented Jun 21, 2019

I'm not entirely sure what these formatting traits should do, as they seem to have been designed with only integers in mind. Would they format as decimals? fractions?

decimal octal decimal octal mixed fraction octal improper fraction
1/8 0.1 1/10 1/10
8/3 2.525252... 2+2/3 10/3

The same questions apply to Binary, Octal, LowerHex and UpperHex. I think LowerExp and UpperExp are going to be a bit more tricky to implement, although they were designed with non-integers in mind, so that's good.

After writing the above table, I'm convinced we should use fractions, as otherwise we'll have infinite repeating decimals all over the place, and we would have to decide how to truncate them. I'm torn between mixed and improper fractions though. There is an argument to be made for mixed fractions, as in the case of a Ratio where the denominator is 1, it necessarily reduces to the integer behavior described in the trait (we could choose to not display denominators of 1 for both fraction methods). Both would be easy to implement, it's just about what users want/expect. Do they want 10000/3 or do they want 3333 + 1/3?

@cuviper
Copy link
Member Author

cuviper commented Jun 26, 2019

We implement Display as decimal improper fractions, so I think other bases would be the same. Display also hides denominators of 1.

FWIW, the current implementation of Num::from_str_radix also only parses numer/denom. We probably should enhance that to parse plain integers too though (without a denominator).

@maxbla
Copy link
Contributor

maxbla commented Jun 27, 2019

I would be happy to submit a PR for this issue, but maybe I'll wait until my other PRs get through. I want to make sure I see those through.

@maxbla maxbla mentioned this issue Jul 25, 2019
4 tasks
@bors bors bot closed this as completed in 47d63e2 Mar 21, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants