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

Human-readable serde impl loses precision, serializes bytes incorrectly #40

Closed
sunshowers opened this issue Jan 7, 2024 · 1 comment · Fixed by #54
Closed

Human-readable serde impl loses precision, serializes bytes incorrectly #40

sunshowers opened this issue Jan 7, 2024 · 1 comment · Fixed by #54
Assignees
Labels
Milestone

Comments

@sunshowers
Copy link

Hi there, thanks for the crate!

While developing against the crate I found that human-readable serde impl's serializer is incorrect. For example, the following program:

use bytesize::ByteSize;

fn main() {
    let json = serde_json::to_string(&ByteSize::mib(1)).unwrap();
    println!("serialized JSON: {json}");

    let deserialized: ByteSize = serde_json::from_str(&json).unwrap();
    println!("deserialized: {}", deserialized.0);
}

prints out:

serialized JSON: "1048.6 KB"
deserialized: 1048600

Which means that serialization/deserialization does not roundtrip.

I think serialization should either be to an integer, or to exact values (e.g. "1048.576 KB"). What do you think?

sunshowers added a commit to oxidecomputer/omicron that referenced this issue Jan 9, 2024
…x serialization (#4781)

This is prep work for #4690. I'd like to add support for making changes
to a DeserializedManifest, then serializing it.

However, it turned out that the bytesize crate does not serialize bytes
correctly (bytesize-rs/bytesize#40). To address
this, just use a u64 and write our own deserializer, using the alternative
parse-size crate.

Also add a test to ensure that serialization of the fake manifest
roundtrips correctly.
@MrCroxx
Copy link
Collaborator

MrCroxx commented Nov 18, 2024

Hi @sunshowers . Thanks for reporting. This was caused by the 1.x uses SI units. With 2.0 (unreleased, I'm going to push the process to release it after introducing some features), bytesize uses IEC units, which would make things clearer for programmers.

@MrCroxx MrCroxx self-assigned this Nov 18, 2024
@MrCroxx MrCroxx added the Q & A label Nov 18, 2024
@MrCroxx MrCroxx added this to the 2.0 milestone Nov 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants