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

Serialises UUID as [u8:16] #331

Merged
merged 8 commits into from Oct 3, 2018
Merged

Conversation

Redrield
Copy link
Contributor

@Redrield Redrield commented Oct 2, 2018

I'm submitting a feature

Description

Adds the ability to serialize a Uuid as a [u8; 16]. Functionality is behind the dense_serde feature. the serde feature continues to serialize as a &[u8]. Both features serialize strings identically

Tests

A new test module for the feature was added, mirroring the tests of the serde feature (with modifications to expected tokens where necessary)

Related Issue(s)

closes #329

* This commit adds a feature `dense_serde` which enables serialization
and deserialization of a Uuid as a [u16; 8]
@kinggoesgaming kinggoesgaming added this to the 0.7.2 milestone Oct 2, 2018
@kinggoesgaming kinggoesgaming added this to In progress in 0.7.x via automation Oct 2, 2018
@kinggoesgaming kinggoesgaming added this to Needs triage in Pull Request Triage via automation Oct 2, 2018
@kinggoesgaming kinggoesgaming moved this from Needs triage to High priority in Pull Request Triage Oct 2, 2018
@Dylan-DPC-zz Dylan-DPC-zz changed the title Fix #329 Serialises UUID as [u8:16] Oct 2, 2018
@KodrAus
Copy link
Member

KodrAus commented Oct 2, 2018

Thanks @Redrield!

From the discussion on #329, using a feature flag here doesn't actually save us from potential breakage. It looks like we want to keep the output of serde the same for anyone that's currently depending on it.

One way we could do this in a backwards compatible way that users properly opt into would be to convert your new Serialize and Deserialize impls into free functions, that can be used with the #[serde(serialize_with)] and #[serde(deserialize_with)] attributes.

@Redrield Redrield changed the title Serialises UUID as [u8:16] [WIP] Serialises UUID as [u8:16] Oct 2, 2018
@kinggoesgaming
Copy link
Member

@KodrAus newtypes?

@smarnach
Copy link

smarnach commented Oct 2, 2018

@kinggoesgaming I think a newtype isn't a natural solution here. It's really the same type, just serialized in a different way, and it may even depend on the context what serialization to use. I like the suggestion to use free functions for use with serde attributes, since it makes the intention of client code immediately obvious.

@Redrield
Copy link
Contributor Author

Redrield commented Oct 2, 2018

I'm thinking add a module that can be used with #[serde(with = "uuid::dense_serde")]. (I'm not sure about the name, just an example)

@KodrAus
Copy link
Member

KodrAus commented Oct 2, 2018

What do you think of uuid::serde::compact? That way we've got a serde module too that any other serde specific stuff can live in?

@kinggoesgaming
Copy link
Member

note uuid::serde namespace can't be used because it conflicts with serde crate

@KodrAus
Copy link
Member

KodrAus commented Oct 3, 2018

Ok, so we could do uuid::adapter::compact, or uuid::adapter::dense instead ?

@Redrield Redrield changed the title [WIP] Serialises UUID as [u8:16] Serialises UUID as [u8:16] Oct 3, 2018
@Redrield
Copy link
Contributor Author

Redrield commented Oct 3, 2018

Rewritten to use serde attributes for dense serialization under uuid::adapter::compact

0.7.x automation moved this from In progress to Needs review Oct 3, 2018
u: &Uuid,
serializer: S,
) -> Result<S::Ok, S::Error> {
println!("sereeeeeeeeealize");
Copy link
Member

Choose a reason for hiding this comment

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

uhmm?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Debug messages, i'll take those out, whoops!

Copy link
Member

Choose a reason for hiding this comment

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

add a // TODO just to be sure :)

we have a bot that marks TODO comments

Copy link
Contributor Author

Choose a reason for hiding this comment

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

wdym? Removed them in b1b2efe

src/adapter/compact.rs Show resolved Hide resolved
0.7.x automation moved this from Needs review to Reviewer approved Oct 3, 2018
@kinggoesgaming
Copy link
Member

bors r+

bors bot added a commit that referenced this pull request Oct 3, 2018
331: Serialises UUID as [u8:16]  r=kinggoesgaming a=Redrield

**I'm submitting a** feature

# Description
Adds the ability to serialize a Uuid as a [u8; 16]. Functionality is behind the `dense_serde` feature. the `serde` feature continues to serialize as a &[u8]. Both features serialize strings identically

# Tests
A new test module for the feature was added, mirroring the tests of the `serde` feature (with modifications to expected tokens where necessary)

# Related Issue(s)
closes #329 


Co-authored-by: Redrield <redrield@gmail.com>
@bors
Copy link
Contributor

bors bot commented Oct 3, 2018

@bors bors bot merged commit b1b2efe into uuid-rs:master Oct 3, 2018
0.7.x automation moved this from Reviewer approved to Done Oct 3, 2018
Pull Request Triage automation moved this from High priority to Closed Oct 3, 2018
@Redrield Redrield deleted the feature/dense_serde branch October 3, 2018 16:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
0.7.x
  
Done
Development

Successfully merging this pull request may close these issues.

Support serialization as [u8; 16] rather than as &[u8]
5 participants