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

Serde like rename for Display trait on enum #216

Open
ModProg opened this issue Nov 9, 2022 · 5 comments
Open

Serde like rename for Display trait on enum #216

ModProg opened this issue Nov 9, 2022 · 5 comments

Comments

@ModProg
Copy link
Contributor

ModProg commented Nov 9, 2022

i.e. I would expect

#[derive(Display)]
#[display(rename="snake_case")]
enum Test{
   First
}

println!("{}", Test::First) // -> prints "first"

The serde behaviour is documented here:
https://serde.rs/container-attrs.html#rename_all

#[serde(rename_all = "...")]

Rename all [...] variants [...] according to the given case convention. The possible values are "lowercase", "UPPERCASE", "PascalCase", "camelCase", "snake_case", "SCREAMING_SNAKE_CASE", "kebab-case", "SCREAMING-KEBAB-CASE".

@JelteF JelteF added this to the 1.1.0 milestone Nov 21, 2022
@JelteF
Copy link
Owner

JelteF commented Nov 21, 2022

This sounds useful. It fits well with the FromStr for unit enum support that was introduced in #176.

bikeshed: I think I prefer rename over rename_all. That way you could also put rename on a single field. The top level rename would then simply set the default for enum items.

@JelteF
Copy link
Owner

JelteF commented Nov 21, 2022

A good question raised in the thread in #225 would be what should be the default? My summary of that discussion is that there are two different usages for which a different default makes sense:

  1. Error types: you probably want a custom error message. By requiring to set this attribute you would not be able to forget to set a custom message for one of your variants.
  2. Any other enum type: You probably want nice display by default

I'd say go for strict by default for now, i.e. error by default. Then if that is deemed too cumbersome we can always relax the default. However, unrelaxing the default would be a breaking change.

@ilslv
Copy link
Contributor

ilslv commented Nov 22, 2022

@JelteF

bikeshed: I think I prefer rename over rename_all. That way you could also put rename on a single field. The top level rename would then simply set the default for enum items.

Actually #[display(rename("...")) is an alias for direct #[display("...")], so I would vote for rename_all as seen in serde and already widespread across the ecosystem.

I'd say go for strict by default for now, i.e. error by default. Then if that is deemed too cumbersome we can always relax the default. However, unrelaxing the default would be a breaking change.

I think that going explicitly over implicitly is a viable option, but only when it's consistent. We already have 2 examples for implicit display:

#[derive(Display)]
struct Unit;

#[derive(Display)]
struct SingleField(u8); // works for named `{ field: u8 }` too

I see implicit Display impl on enums with unit fields as understandable and discoverable continuation for that:

#[derive(Display)]
enum Enum {
    A,
    B,
}

Moreover this behaviour is already usable on master (and released as well):

// derive_more::From fits nicely into this pattern as well
#[derive(Debug, Display, Error, From)]
enum CompoundError {
    Simple,
    WithSource {
        source: Simple,
    },
    #[from(ignore)]
    WithBacktraceFromSource {
        #[error(backtrace)]
        source: Simple,
    },
    #[display(fmt = "{source}")]
    WithDifferentBacktrace {
        source: Simple,
        backtrace: Backtrace,
    },
    WithExplicitSource {
        #[error(source)]
        explicit_source: WithSource,
    },
    #[from(ignore)]
    WithBacktraceFromExplicitSource {
        #[error(backtrace, source)]
        explicit_source: WithSource,
    },
    Tuple(WithExplicitSource),
    WithoutSource(#[error(not(source))] Tuple),
}

@marcocondrache
Copy link

Any update on this issue? Applying a pre processing function on every variant name is very useful in different situations.

@JelteF
Copy link
Owner

JelteF commented Jan 3, 2024

Okay this dropped off my radar.

@marcocondrache at the moment we're focussing on tasks that are blockers of 1.0.0, this is not one of them, because it can be implemented in a backwards compatible way. Feel free to contribute a PR that implements this.

@ilslv Sorry I didn't respond. Your response make total sense. Let's go with rename_all and implicitely doing it (because we already do that and it doesn't seem worth breaking backwards compatibility for this).

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

No branches or pull requests

4 participants