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

Allow representation to differ between human-readable and binary formats #790

Closed
dtolnay opened this issue Feb 26, 2017 · 6 comments
Closed

Comments

@dtolnay
Copy link
Member

dtolnay commented Feb 26, 2017

As an initial brainstormed approach to give an example of what I have in mind:

trait Serializer {
    /* existing associated types and methods */

    fn serialize_readable_or_compact<T>(self, t: T) -> Result<Self::Ok, Self::Error>
        where T: SerializeReadable + SerializeCompact;
}

trait SerializeReadable { /* just like Serialize */ }
trait SerializeCompact { /* just like Serialize */ }

... and similar for deserialization.

The idea is that chrono::DateTime, std::net::IpAddr etc can serialize themselves as a string in JSON and YAML but compact bytes in Bincode. Currently a SocketAddrV6 takes up to 55 bytes to represent in Bincode but should only be 18 bytes.

@clarfonthey
Copy link
Contributor

I think I'd describe data in this form as a "blob;" formats like JSON can only store arbitrary data as strings, whereas formats like bincode can store any arbitrary bytes. I have an idea for how I'd implement this and I'll post it later once I write it up.

@clarfonthey
Copy link
Contributor

clarfonthey commented Feb 26, 2017

Perhaps like this?

trait Serializer {
    // existing stuff
    fn serialize_blob<T>(self, t: T) -> Result<Self::Ok, Self::Error> where T: SerializeBlob;
}

trait SerializeBlob: Display {
    fn fmt_bytes<W>(&self, writer: W) -> io::Result<()> where W: io::Write {
        write!(writer, "{}", self)
    }
}

I'm open for ideas on how to do this without requiring libstd. I'd rather it not be too complicated, though.

@dtolnay
Copy link
Member Author

dtolnay commented Feb 26, 2017

What do you see as the advantages of that approach? Here are some disadvantages:

  • If you are calling serialize_blob, you want the representation to be different in bytes form vs string form so the default implementation of fmt_bytes is never what you want. Otherwise you would just use collect_str instead of serialize_blob.
  • Only supports string/byte representations rather than a more general concept of readable and compact, which may be a non-string in the readable case (for example a map) and non-bytes in the compact case (for example a tuple of integers for an IP address).

@dtolnay
Copy link
Member Author

dtolnay commented Mar 6, 2017

Of course here is the KISS approach:

trait Serializer {
    /* existing associated types and methods */

    #[inline]
    fn is_human_readable() -> bool { true }
}
fn serialize<S>(&self, serializer: S) -> Result<S::Ok, S::Error>
    where S: Serializer
{
    if S::is_human_readable() {
        serializer.collect_str(self)
    } else {
        serializer.serialize_bytes(self.as_bytes())
    }
}

@clarfonthey
Copy link
Contributor

I like it, although I think that it'd be nice to have a method that enables the user to test both branches without having to change anything in the serializer.

Perhaps we could add a separate serialize_binary method that defaults to delegating to serialize and is called by the serializer if appropriate?

Marwes pushed a commit to Marwes/serde_state that referenced this issue Sep 7, 2017
This implements the KISS suggested in serde-rs#790.
It is possible that one of the other approaches may be better but this
seemed like the simplest one to reignite som discussion.

Personally I find the original suggestion of adding two traits perhaps slightly
cleaner in theory but I think it ends up more complicated in the end
since the added traits also need to be duplicated to to the `Seed`
traits.

Closes serde-rs#790
Marwes pushed a commit to Marwes/serde_state that referenced this issue Sep 7, 2017
This implements the KISS suggested in serde-rs#790.
It is possible that one of the other approaches may be better but this
seemed like the simplest one to reignite som discussion.

Personally I find the original suggestion of adding two traits perhaps slightly
cleaner in theory but I think it ends up more complicated in the end
since the added traits also need to be duplicated to to the `Seed`
traits.

Closes serde-rs#790
@Marwes
Copy link
Contributor

Marwes commented Sep 7, 2017

@clarcharr

I like it, although I think that it'd be nice to have a method that enables the user to test both branches without having to change anything in the serializer.
Perhaps we could add a separate serialize_binary method that defaults to delegating to serialize and is called by the serializer if appropriate?

Both Serializer and Serialize implementations still need to be modified to support this case though so there is not really anything gained from that approach?

dtolnay pushed a commit to serde-rs/test that referenced this issue Jul 26, 2023
This implements the KISS suggested in serde-rs/serde#790.
It is possible that one of the other approaches may be better but this
seemed like the simplest one to reignite som discussion.

Personally I find the original suggestion of adding two traits perhaps slightly
cleaner in theory but I think it ends up more complicated in the end
since the added traits also need to be duplicated to to the `Seed`
traits.

Closes #790
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

No branches or pull requests

3 participants