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

Optionally always include float decimals #237

Merged
merged 1 commit into from
May 26, 2020
Merged

Conversation

cart
Copy link
Contributor

@cart cart commented May 23, 2020

This adds the option to always include the decimal point in floats.

Currently ron always serializes 5.0 as 5. I have a use case that makes the 5 vs 5.0 distinction important. My rust type is a dynamic collection that can contain either floats or ints. Round tripping a value of 5.0 with ron results in an integer output of 5 when its source was a float.

@cart
Copy link
Contributor Author

cart commented May 23, 2020

Also, I'd love to use rust's fmt syntax to tack on the .0, but I can't find a way to use the minimum amount of precision required. Ex: write!(w, "{:.5}", 1.0) will always include the decimal, but it will always use a precision of exactly 5 decimal places: 1.00000. I also don't think rounding here is desirable.

Copy link
Collaborator

@kvark kvark left a comment

Choose a reason for hiding this comment

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

Thank you! I think this is a great feature, and I'd be happy to use it once available (presumably, in 0.6.1).
I have a few small concerns.
Also please consider adding a CHANGELOG entry for this.

src/ser/mod.rs Outdated
@@ -363,11 +388,17 @@ impl<'a, W: io::Write> ser::Serializer for &'a mut Serializer<W> {

fn serialize_f32(self, v: f32) -> Result<()> {
write!(self.output, "{}", v)?;
if self.decimal_floats() && v - v.floor() == 0.0 {
Copy link
Collaborator

Choose a reason for hiding this comment

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

why isn't this written as v == v.floor()?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No reason. Your representation is clearly better 😄

tests/floats.rs Outdated
#[test]
fn decimal_floats() {
let pretty = PrettyConfig::new().with_decimal_floats(false);
let without_decimal = to_string_pretty(&1.0, pretty).expect("Serialization failed");
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we need those expect strings here? For tests, I think unwrap is perfectly fine.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was following the pattern in the "preserve_sequence" tests, but I'm happy to swap these out. "preserve_sequence" appears to be the only existing test that uses expect.

@cart cart force-pushed the decimal_floats branch 2 times, most recently from 33c5539 to d9a8305 Compare May 26, 2020 16:02
Copy link
Collaborator

@kvark kvark left a comment

Choose a reason for hiding this comment

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

Thank you!

@kvark
Copy link
Collaborator

kvark commented May 26, 2020

Note: clippy isn't happy with float comparisons (red on CI)

@cart
Copy link
Contributor Author

cart commented May 26, 2020

i opted for the clippy recommended solution: (v - v.floor()).abs() < f32::EPSILON

@cart
Copy link
Contributor Author

cart commented May 26, 2020

oops :)

@cart
Copy link
Contributor Author

cart commented May 26, 2020

It looks like EPSILON associated constants aren't in 1.34. Is backward compat desirable? I can either inline the epsilon values or someone will need to bump that version to 1.43. What should I do here?

@kvark
Copy link
Collaborator

kvark commented May 26, 2020

Let's not raise the Rust version just yet. Debian and other distros are typically much behind the top of the tree.

@kvark kvark merged commit b43c107 into ron-rs:master May 26, 2020
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