Skip to content

Support (de)serialize_with in tuples #335

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

Merged
merged 1 commit into from
May 23, 2016
Merged

Support (de)serialize_with in tuples #335

merged 1 commit into from
May 23, 2016

Conversation

dtolnay
Copy link
Member

@dtolnay dtolnay commented May 16, 2016

Fixes #330 and a number of related bugs around (de)serialize_with in newtype structs, tuple structs, newtype variants, and tuple variants.

The most interesting thing in this PR is a significantly condensed version of wrap_serialize_with. It now works a lot more like wrap_deserialize_with, with just a thin wrapper that provides a serialize method. Please confirm that the contortions in the original implementation were not necessary. I had to make this change to support serialize_with in newtype structs and newtype variants.

@@ -177,31 +177,29 @@ pub struct FieldAttrs {
name: Name,
skip_serializing_field: bool,
skip_deserializing_field: bool,
skip_serializing_field_if: Option<P<ast::Expr>>,
skip_serializing_if: Option<ast::Path>,
Copy link
Member

Choose a reason for hiding this comment

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

iirc the idea was to allow arbitrary expressions in the future

Copy link
Member Author

Choose a reason for hiding this comment

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

I see. Given that it's not on our roadmap (no issue for it) and I am not convinced that it would be a valuable thing to support, I do no think we should keep around a lot of convoluted dead code in the hope that some day it will be useful.

@dtolnay
Copy link
Member Author

dtolnay commented May 19, 2016

I rebased to fix the merge conflict with #338. We both added something to the bottom of serde_tests/tests/test_gen.rs.

@dtolnay
Copy link
Member Author

dtolnay commented May 19, 2016

The travis failure is clippy doesn't build.

@oli-obk
Copy link
Member

oli-obk commented May 23, 2016

I think I grokked most of the code now.

I am not convinced that it would be a valuable thing to support, I do no think we should keep around a lot of convoluted dead code in the hope that some day it will be useful.

agreed on both points. We should probably add cfail tests some day to test for behavior we don't want.

@homu r+

homu added a commit that referenced this pull request May 23, 2016
Support (de)serialize_with in tuples

Fixes #330 and a number of related bugs around (de)serialize_with in newtype structs, tuple structs, newtype variants, and tuple variants.

The most interesting thing in this PR is a significantly condensed version of `wrap_serialize_with`. It now works a lot more like `wrap_deserialize_with`, with just a thin wrapper that provides a `serialize` method. Please confirm that the contortions in the original implementation were not necessary. I had to make this change to support serialize_with in newtype structs and newtype variants.
@oli-obk
Copy link
Member

oli-obk commented May 23, 2016

hmmm right... well... it looks like clippy is going to work again soon, I'll postpone instead of merging manually

@dtolnay
Copy link
Member Author

dtolnay commented May 23, 2016

We should probably add cfail tests some day to test for behavior we don't want.

I filed #341 to add cfail tests.

The clippy PR we need to watch is https://github.com/Manishearth/rust-clippy/pull/944, going to be published as 0.0.69.

@mcarton
Copy link

mcarton commented May 23, 2016

@dtolnay done

@dtolnay
Copy link
Member Author

dtolnay commented May 23, 2016

Merging because I retriggered the Travis build and it passed.

@dtolnay dtolnay merged commit ea182e2 into serde-rs:master May 23, 2016
@dtolnay dtolnay deleted the tuples branch May 23, 2016 20:34
@dtolnay dtolnay added this to the v0.7.6 milestone Jun 9, 2016
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.

3 participants