Skip to content

Conversation

@msrd0
Copy link
Contributor

@msrd0 msrd0 commented Mar 29, 2025

This PR allows (de)serializing rationals using serde. If the rational is representable as an integer, we serialize it as such, otherwise as a string displaying the fraction.

msrd0 added 2 commits March 29, 2025 12:12
we're doing this because some formats such as `toml`
decided to not support i128
@ijagberg
Copy link
Owner

Thanks for your PR!

Would you mind adding a few unit tests? (De)serializing an integer, a non-integer, and very small/large values would be nice to see.

@msrd0
Copy link
Contributor Author

msrd0 commented Mar 30, 2025

Sure, I added some unit tests.

@ijagberg
Copy link
Owner

Great, thank you!

As I mentioned in the issue you posted, I have some qualms about changing the serialization behavior based on the value of the rational. It would seem surprising to me as a user of the library if Rational::new(1, 2) serialized to a string, but Rational::new(5, 1) serialized to an integer. Why not just always serialize to a string? Rational::new(5, 1) would become "5/1". That seems fine to me, and if I was writing something that was parsing these JSON values I would only need to handle JSON strings, and not worry about first checking if the value is an integer or not. This would also maybe help with the arbitrary precision stuff you wrote about?

We could still support deserializing from both integers and strings however, that seems like a reasonable feature to include.

@msrd0
Copy link
Contributor Author

msrd0 commented Apr 1, 2025

I changed it so that it always serializes as a string.

This would also maybe help with the arbitrary precision stuff you wrote about?

No, that's just serde_json being stupid in handling very big numbers. Which, given how loosely json defines what a "number" is, maybe shouldn't be surprising. But as I wrote, using serde_json::Value for the tests ended up working as expected.

@ijagberg
Copy link
Owner

ijagberg commented Apr 2, 2025

Gotcha. Anyways, thanks for the contribution, I'll merge this and release it in a sec.

@ijagberg ijagberg merged commit 500c165 into ijagberg:main Apr 2, 2025
1 check passed
@ijagberg
Copy link
Owner

ijagberg commented Apr 2, 2025

Released version 1.7.0

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