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

Adds serializer format specific field names #69

Merged
merged 13 commits into from May 15, 2015

Conversation

hugoduncan
Copy link
Contributor

Allows different field names to be used for different external formats.

Field names are specified using the rename field attribute, e.g:

#[serde(rename(xml= "a4", json="a5"))]

The implementation adds a fmt method to the Serializer and
Deserializer traits, allowing them to name the format that
produce/consume.

If no format specific renaming is used, the code produced should be the
same as before this change.

The code does not allow for a global rename and a format specific rename
at the same time, so either you use the global rename, or you have to
explicitly add a rename for each format used.

This still needs to be completed to correctly report missing field names
as per 1748831.

Reverts #62

Addresses #61

@hugoduncan
Copy link
Contributor Author

@erickt Is it possible that this will be accepted in some form? If so I'll finish it up.

@@ -113,6 +113,10 @@ pub trait Deserializer {
{
self.visit(visitor)
}

fn fmt() -> &'static str {
Copy link
Member

Choose a reason for hiding this comment

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

Could this be renamed to format()?

@erickt
Copy link
Member

erickt commented Apr 29, 2015

Why did you revert #62?

.zip(value_exprs)
.enumerate()
.map(|(i, (key_expr, value_expr))| {
.map(|(i, (field, value_expr))| {
let key_expr = match field {
Copy link
Member

Choose a reason for hiding this comment

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

Could this logic be moved into field.rs?

@hugoduncan
Copy link
Contributor Author

I implemented this as #62 ended up being insufficient, in particular due to the lack of round-tripping when it was used (and I wanted to use json with rust field names, as well as XML and urlencoded query arguments).

I don't have a use case for different field names on serialisation and deserialisation in the same format.

Supporting both this and #62 would complicate the code.

Having said that, I am willing to put #62 back into this code if you think it should stay.

@erickt
Copy link
Member

erickt commented Apr 29, 2015

Overall I like what you're doing here. The one other general thought I have is how we should grow this attribute grammar if we get some other format-specific attributes. For example, say we wanted to annotate if a field should be an xml attribute or an element. Would it be better then if we supported:

struct Person {
     #[serde(xml(attribute, rename="Gender"), json(rename="Gender"))]
     gender: String,

     #[serde(xml(element, rename="Name"), json(rename="Gender"))]
     name String,
}

To produce:

<person Gender="female">
    <Name>Emily</Name>
</person>

or:

{
  "Gender": "female",
  "Name": "Emily"
}

?

@erickt
Copy link
Member

erickt commented Apr 29, 2015

@hugoduncan: I wasn't crazy about the lack of round-trip-ability of #62, so if you don't need it, then I'm fine pulling it out until someone else asks for it :)

@hugoduncan
Copy link
Contributor Author

I think another question is whether format independent attributes are required.

struct Person {
     #[serde(rename="Gender", formats(xml(attribute, rename="XmlGender"))]
     gender: String,
}

@hugoduncan
Copy link
Contributor Author

@erickt I have refactored things to address your comments.

The attribute grammer is the remaining issue.

quasi = "*"
quasi_macros = "*"

[dependencies.quasi]
Copy link
Member

Choose a reason for hiding this comment

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

I don't think this is necessary anymore.

@hugoduncan hugoduncan force-pushed the feature/format-specific-field-names branch from 0d68d66 to ff2b088 Compare May 7, 2015 21:42
@hugoduncan
Copy link
Contributor Author

Oops, looks like I pushed a couple of commits to this branch in error (the XML related changes). I've removed these.

@hugoduncan
Copy link
Contributor Author

As a general question (for quasia and aster too), should this be targeting 1.0, or 1.1-nightly?

@erickt
Copy link
Member

erickt commented May 8, 2015

@hugoduncan: looks like the tests failed with an odd error:

examples/json.rs:11:28: 11:39 error: unexpected token: `,`
examples/json.rs:11 #[derive(Debug, Serialize, Deserialize)]

Any idea what could be causing that?

On quasi and aster, they need to be nightly, but do try to stay close to 1.0 otherwise. I'm close to getting syntex working, which would let us use serde_macros on stable :)

Allows different field names to be used for different external formats.

Field names are specified using the `rename` field attribute, e.g:

    #[serde(rename(xml= "a4", json="a5"))]

Reverts serde-rs#62

Addresses serde-rs#61
@hugoduncan
Copy link
Contributor Author

@erickt I'm still trying to pin it down (and am having trouble building a simple repro case), but quasi seems to have started disliking ,s between (and after the last) match arms, and also in one case rejecting a , between two expressions used as arguments in a function call expression.

@hugoduncan hugoduncan force-pushed the feature/format-specific-field-names branch from 407cdc0 to 801f37b Compare May 14, 2015 21:40
@hugoduncan
Copy link
Contributor Author

Fixed compilation issues, and rebased. Looks like travis timed out the compilation.

erickt added a commit that referenced this pull request May 15, 2015
…names

Adds serializer format specific field names
@erickt erickt merged commit ee45eb8 into serde-rs:master May 15, 2015
@hugoduncan hugoduncan deleted the feature/format-specific-field-names branch May 15, 2015 12:13
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