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

Add prefix_all attribute #1426

Closed
wants to merge 1 commit into from
Closed

Conversation

Phlosioneer
Copy link

@Phlosioneer Phlosioneer commented Nov 22, 2018

The prefix_all attribute will add a given prefix to all the fields in
a struct, or all the variants in an enum. It is applied
after rename_all and respects rename.

This feature is incredibly useful when dealing with enum encodings that prefix each variant with the name of the enum. For example, in some work I'm doing:

{
  "result": "NetworkResult_Success"
}

I may not have chosen the best way to add the prefix to a string. It required an extra allocation per rename. I did my best to keep it efficient.

I ran clippy, and added tests. I'm also opening a corresponding PR in the book's repo to add documentation for it. If I've forgotten anything, let me know, and I'll do my best to reply promptly.

The prefix_all attribute will add a given prefix to all the fields in
a struct, or all the variants in an enum. It is applied
after `rename_all` and respects `rename`.
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! The implementation looks good.

Would there be any way to support this without adding a new attribute to serde_derive? I feel like this one is likely to be somewhat less commonly useful than the existing attributes.

For example you could implement it as an attribute macro that inserts the right serde(rename = "...") attributes:

#[serde_prefix_all = "NetworkResult_"]
#[derive(Deserialize)]
enum NetworkResult {
    Success,
}

Or extend serde_with::with_prefix! to support prefixing variant names.

#[derive(Deserialize)]
struct Packet {
    #[serde(with = "prefix_network_result")]
    result: NetworkResult,
}

#[derive(Deserialize)]
enum NetworkResult {
    Success,
}

// Expand to a serialize_with and deserialize_with function with the given name.
with_prefix!(prefix_network_result "NetworkResult_");

In the case of the first approach, we could potentially add some tricks to make helpers provided in third party crates feel more seamless.

extern crate serde_prefix_all;
use serde_prefix_all::prefix_all;

#[derive(Deserialize)]
#[serde(apply = "prefix_all", prefix = "NetworkResult_")] // expands to #[prefix_all(prefix = "NetworkResult_")]
enum NetworkResult {
    Success,
}

@Phlosioneer
Copy link
Author

I had no idea serde_with was a thing at all. I didn't think it was possible to extend serde_derive as a 3rd party crate.

As to your point: I think it's fairly common. But I understand. It was far, far easier for me to extend serde_derive than to make a custom proc macro; and I couldn't figure out how to wrangle the deserialize trait to be anything resembling "compose-able", so that wasn't an option. If you don't want it, I can just keep my patched version.

I will say this: without this feature, prefix systems are awful. I had ~140 #[serde(rename = "...")] that were just prefixes.

@dtolnay
Copy link
Member

dtolnay commented Nov 24, 2018

As to your point: I think it's fairly common.

It may be common but I am still a bit skeptical because the only previous request for a prefix_all sort of thing would not have been addressed by this change. The use case is described here. They needed to use different prefixes across different uses of the same type.

I am closing the PR because I would like to move in the direction of making serde_derive integrate better with logic found in external libraries like serde_with and serde_aux rather than adding more logic into serde_derive. I would love your help on that sort of design.

Thanks anyway!

@dtolnay dtolnay closed this Nov 24, 2018
@Phlosioneer
Copy link
Author

I'd be willing to help set that up. I've gotten much more familiar with the design. If something like this could easily be written as a plugin sort of thing, it would be just as good. I think it's also important to suggest ways to extend serde's functionality in the documentation, so that more people will think to search for crates like serde_with and serde_aux.

Is there an issue for discussion about options?

@dtolnay
Copy link
Member

dtolnay commented Nov 24, 2018

Filed as #1427.

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.

2 participants