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

Serialize to binary if the serde format is not human readable #104

Merged
merged 2 commits into from
Nov 2, 2017

Conversation

Marwes
Copy link
Contributor

@Marwes Marwes commented Nov 2, 2017

This uses the
is_human_readable
method added in serde 1.0.16 to serialize uuid's in a more compact
form if the serialized format does not need to be human readable.

This does not have an explicit test for is_human_readable == false since serde-rs/test#12 has not been completed for serde_test so you may want to wait with merging. I have verified it manually though.

(Maybe?) related serde-rs/serde#90

Markus Westerlind added 2 commits November 1, 2017 13:13
This uses the
[is_human_readable](https://github.com/serde-rs/serde/releases/tag/v1.0.16)
method added in serde 1.0.16 to serialize uuid's in a more compact
form if the serialized format does not need to be human readable.

(Maybe) related uuid-rs#90

BREAKING CHANGE

If serde serialization is used with non-human-readable format then
uuid's will be serialized as bytes instead of strings
@alexcrichton
Copy link
Contributor

Awesome, thanks! I wonder though, is this actually a breaking change? If the serialization changed for IpAddr then I'd imagine that this would be able to change as well, right?

@Marwes
Copy link
Contributor Author

Marwes commented Nov 2, 2017

Ah, right. It is only for formats setting is_human_readable == false that this is a breaking change as the default value of true will result in the same serialization (as reasoned in serde-rs/serde#1044 (comment))

@alexcrichton
Copy link
Contributor

Ok! In that case I'm gonna merge this as it looks great!

@alexcrichton alexcrichton merged commit e1fc8cc into uuid-rs:master Nov 2, 2017
@Marwes Marwes deleted the human_readable branch November 2, 2017 18:50
Marwes pushed a commit to Marwes/uuid that referenced this pull request Nov 7, 2017
Ended up completing the compact/readable serde_test API so here are the
tests missing from uuid-rs#104.

I also replaced the `serde_json` roundtrip test with plain `assert_tokens`
tests which do essentially the same thing. I can re-add it if it seems
worthwhile to keep it though.
@Marwes
Copy link
Contributor Author

Marwes commented Nov 20, 2017

Could we get a crates.io release with this?

(Is there a better way to request/indicate a crates.io release in stable/slow moving crates such as this?)

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.

3 participants