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

implement (de)serialize_with for variants #1015

Merged
merged 3 commits into from Sep 9, 2017

Conversation

spinda
Copy link
Contributor

@spinda spinda commented Aug 8, 2017

Work in progress for #1013. So far I've implemented the serialize_with half. Any early feedback would be appreciated.

@dtolnay dtolnay added the wip label Aug 8, 2017
Copy link
Member

@dtolnay dtolnay left a comment

Choose a reason for hiding this comment

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

Looks great so far!

#[serde(serialize_with="serialize_some_other_variant")]
Struct {
f1: String,
#[serde(skip_serializing_if="is_zero")]
Copy link
Member

Choose a reason for hiding this comment

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

I would prefer not to support skip attributes inside of a variant with serialize_with until somebody has a compelling use case for it. For now the derive should panic so it fails to compile. I feel like it is just as convenient and more obvious for this logic to be encompassed inside the serialize_with function:

fn serialize_some_other_variant<S>(f1: &String,
                                   f2: &u8,
                                   serializer: S)
                                   -> Result<S::Ok, S::Error>
    where S: Serializer
{
    if is_zero(*f2) {
        /* ... */
    } else {
        /* ... */
    }
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, I'll rip that part out. I was split on this and ended up erring on the side of compatibility with existing features.

As discussed in serde-rs#1013, serialize_with functions attached to variants receive an
argument for each inner value contained within the variant. Internally such a
function is wired up to the serializer as if the variant were a newtype variant.
Complements variant serialize_with and closes serde-rs#1013.
@spinda spinda changed the title (wip) implement (de)serialize_with for variants implement (de)serialize_with for variants Aug 14, 2017
@spinda
Copy link
Contributor Author

spinda commented Aug 14, 2017

@dtolnay That should wrap up the implementation (no longer WIP).

@oli-obk
Copy link
Member

oli-obk commented Aug 15, 2017

The clippy thing is unfortunate. @dtolnay do you think it would make sense to allow derefs in serialize_with and deserialize_with? We'd have to generate an intermediate function/closure at the expanded call site, but that should be doable.

@dtolnay dtolnay removed the wip label Aug 16, 2017
@dtolnay
Copy link
Member

dtolnay commented Aug 16, 2017

We already allow derefs in serialize_with and deserialize_with. That is why #631 (comment) works, for example. The tests added in this PR can be changed as clippy says: use &str instead of &String.

The generated code is doing the equivalent of this, which works:

struct __SerializeWith<'__a> {
    values: (&'__a String, &'__a u8),
}

fn main() {
    let sw = __SerializeWith { values: (&"".to_owned(), &0) };
    serialize_some_other_variant(sw.values.0, sw.values.1);
}

fn serialize_some_other_variant(_: &str, _: &u8) {}

@spinda
Copy link
Contributor Author

spinda commented Aug 16, 2017

Oh, so you can. I didn't realize that. I've pushed a fix.

Copy link
Member

@dtolnay dtolnay left a comment

Choose a reason for hiding this comment

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

Thanks, this is amazing work. I apologize that it took so long to review fully.

@dtolnay dtolnay merged commit 15b2714 into serde-rs:master Sep 9, 2017
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

3 participants