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

Serde support #32

Closed
sffc opened this issue Feb 10, 2021 · 12 comments · Fixed by #33
Closed

Serde support #32

sffc opened this issue Feb 10, 2021 · 12 comments · Fixed by #33

Comments

@sffc
Copy link
Collaborator

sffc commented Feb 10, 2021

@gregtatum said in unicode-org/icu4x#480 that he can't put a TinyStr into a serde struct field because TinyStr doesn't support serde. We should add serde to TinyStr.

We could make the serialized representation either an integer or a string. I favor a string because it is more human-readable. In certain serialization forms, storing the integer form may be a bit smaller, either because you can omit the "" or the string length header, but I don't think that's worth the readability. On the other hand, we could allow both forms and have a serde attribute that lets you switch between them when serializing.

@zbraniecki
Copy link
Owner

Can we do both? The advantage of int is zero cost deseralizing.

@sffc
Copy link
Collaborator Author

sffc commented Feb 10, 2021

Can we do both?

We could, if we think it's useful.

The advantage of int is zero cost deseralizing.

It's not zero-cost, because we still have to validate it.

@sffc
Copy link
Collaborator Author

sffc commented Feb 10, 2021

Another advantage of strings is that it would make it friendlier when consuming serialized data from other platforms that don't have TinyStr. The integer representation is sort-of internal to TinyStr in Rust.

@sffc
Copy link
Collaborator Author

sffc commented Feb 10, 2021

Thoughts on starting with string serialization, and considering it a feature request to add integer serialization?

@gregtatum
Copy link

starting with string serialization, and considering it a feature request to add integer serialization

I agree with this approach.

@zbraniecki
Copy link
Owner

We can have an unchecked deserialize from int. But i agree we can start with string!

@Manishearth
Copy link
Collaborator

This works for me!

@Manishearth
Copy link
Collaborator

I can make a quick PR

@Manishearth
Copy link
Collaborator

We can use Serializer::is_human_readable to serialize to both strings or integers depending on what the serializer type is

@sffc
Copy link
Collaborator Author

sffc commented Feb 11, 2021

I like that! If we do it, I would still prefer to validate the integers when deserializing, because data can come from unknown sources. If we want to do unchecked deserialization as Zibi mentioned above, that should be an opt-in mode.

@zbraniecki
Copy link
Owner

If we want to do unchecked deserialization as Zibi mentioned above, that should be an opt-in mode.

Sure, I agree. I believe that in environments where both input and output is guaranteed to be cohesive, the huge win of TinyStr is that it is an int that can be zero-cost deserialized. I'm ok with it being opt-in.

@Manishearth
Copy link
Collaborator

I like that! If we do it, I would still prefer to validate the integers when deserializing, because data can come from unknown sources. If we want to do unchecked deserialization as Zibi mentioned above, that should be an opt-in mode.

Oh yeah we can never do this unchecked, but either way we're validating, this way we can validate a little less 😄

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 a pull request may close this issue.

4 participants