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

Inform serializers about skipped fields. #1022

Merged
merged 3 commits into from
Sep 8, 2017

Conversation

sfackler
Copy link
Contributor

Closes #960.

@dtolnay
Copy link
Member

dtolnay commented Aug 21, 2017

I would like to see a cbor PR working with this before I would merge it. I am on board with this idea but it would be tough to undo later if it turns out we are missing something...

Have you looked into how this interacts with unconditionally skipped fields?

#[derive(Serialize, Deserialize)]
struct Sfackler {
    a: u8,
    #[serde(skip)]
    b: u8,
    c: u8,
}

It looks like Serialize will call serialize_field("a"), skip_field("b"), serialize_field("c") so presumably "c" would be serialized with index 2, but then Deserialize invokes deserialize_struct("Sfackler", &["a", "c"]) in which there is no field with index 2.

@sfackler
Copy link
Contributor Author

sfackler commented Sep 4, 2017

Yeah, the behavior for unconditionally skipped fields is wrong.

I'm not sure this is sufficient to handle compact struct field names - we'd also need to support numeric deserialization of field identifiers.

@dtolnay
Copy link
Member

dtolnay commented Sep 4, 2017

We used to have numeric deserialization of field identifiers but got rid of it because nobody ever used it. I would be okay putting it back.

@sfackler
Copy link
Contributor Author

sfackler commented Sep 6, 2017

I've fixed the handling of skipped fields and enabled identifier deserialization from u32.

Here's an example of use for CBOR packed serialization: sfackler/cbor@d2409da

@dtolnay dtolnay merged commit 800620e into serde-rs:master Sep 8, 2017
@dtolnay
Copy link
Member

dtolnay commented Sep 8, 2017

Thanks for persisting with this ❤️. I will try to squeeze in some of the other reviews tonight and get this released no later than tomorrow night.

@sfackler
Copy link
Contributor Author

sfackler commented Sep 8, 2017

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants