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.

Sign up for GitHub

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

Jump to bottom

Serde like rename for Display trait on enum #216

Open
ModProg opened this issue Nov 9, 2022 · 7 comments · May be fixed by #443
Open

Serde like rename for Display trait on enum #216

ModProg opened this issue Nov 9, 2022 · 7 comments · May be fixed by #443
Labels
enhancement help wanted
Milestone
2.1.0

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".
@tyranron tyranron mentioned this issue Nov 18, 2022
Merged
3 tasks
@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),
}

@ilslv ilslv mentioned this issue Nov 22, 2022
Closed
@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 •
edited
Loading

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).

@ysndr ysndr mentioned this issue Jan 29, 2025
Merged
ysndr added a commit to flox/flox that referenced this issue Jan 29, 2025
@ysndr
squash! feat(catalog): add an enum for Quality of Service classes
68cfb01 
`display(rename_all = "snake_case")` does not actually seem to be a thing,
that is supported by `derive_more` [1].
Here, for sake of simplicity, i manually named the fields.
Alternatively, we could change the "extra_headers" to `Map<String, serde_json::Value>`,
which would then allow using the `Serialize` derive here as intended.

[1]: <JelteF/derive_more#216>
ysndr added a commit to flox/flox that referenced this issue Jan 30, 2025
@ysndr
feat(catalog): add an enum for Quality of Service classes
b25edd8 
`CatalogQoS` is represents the QoS classes exposed by macos
and described for example in [1].
As of now we don't have a usecase for the majority of these,
and will start with only using `background` for `upgrade`/`resolve`
requests made by the `check_for_upgrade` command,
which runs ... in the background.
Additional classes are included to provide a "proven" framework
for QoS classes in full.

[1]: <https://blog.xoria.org/macos-tips-threading/>

fix: provide display name for each class manually

`display(rename_all = "snake_case")` does not actually seem to be a thing,
that is supported by `derive_more` [1].
Here, for sake of simplicity, i manually named the fields.
Alternatively, we could change the "extra_headers" to `Map<String, serde_json::Value>`,
which would then allow using the `Serialize` derive here as intended.

[1]: <JelteF/derive_more#216>
github-merge-queue bot pushed a commit to flox/flox that referenced this issue Jan 30, 2025
@ysndr
feat(catalog): add an enum for Quality of Service classes
173c1ed 
`CatalogQoS` is represents the QoS classes exposed by macos
and described for example in [1].
As of now we don't have a usecase for the majority of these,
and will start with only using `background` for `upgrade`/`resolve`
requests made by the `check_for_upgrade` command,
which runs ... in the background.
Additional classes are included to provide a "proven" framework
for QoS classes in full.

[1]: <https://blog.xoria.org/macos-tips-threading/>

fix: provide display name for each class manually

`display(rename_all = "snake_case")` does not actually seem to be a thing,
that is supported by `derive_more` [1].
Here, for sake of simplicity, i manually named the fields.
Alternatively, we could change the "extra_headers" to `Map<String, serde_json::Value>`,
which would then allow using the `Serialize` derive here as intended.

[1]: <JelteF/derive_more#216>
github-merge-queue bot pushed a commit to flox/flox that referenced this issue Jan 30, 2025
@ysndr
feat(catalog): add an enum for Quality of Service classes
b5efe1f 
`CatalogQoS` is represents the QoS classes exposed by macos
and described for example in [1].
As of now we don't have a usecase for the majority of these,
and will start with only using `background` for `upgrade`/`resolve`
requests made by the `check_for_upgrade` command,
which runs ... in the background.
Additional classes are included to provide a "proven" framework
for QoS classes in full.

[1]: <https://blog.xoria.org/macos-tips-threading/>

fix: provide display name for each class manually

`display(rename_all = "snake_case")` does not actually seem to be a thing,
that is supported by `derive_more` [1].
Here, for sake of simplicity, i manually named the fields.
Alternatively, we could change the "extra_headers" to `Map<String, serde_json::Value>`,
which would then allow using the `Serialize` derive here as intended.

[1]: <JelteF/derive_more#216>
@ModProg
Copy link
Contributor Author

ModProg commented Feb 2, 2025 •
edited
Loading

rename_all should only be available on enums, right? I.e. not allow renaming fields when deriving Debug on a struct.

Also should this only be available for Display or all fmt::* derive attributes. (looking at the code it'd probably be more effort to separate this behavior between fmt traits, as they share quite a bit code).

@ModProg ModProg linked a pull request Feb 2, 2025 that will close this issue
Open
3 tasks
ModProg added a commit to ModProg/derive_more that referenced this issue Feb 2, 2025
@ModProg
impl rename_all for #[derive(Display)] (JelteF#216)
f41c1dd
@ModProg
Copy link
Contributor Author

ModProg commented Feb 2, 2025

Took a shot at implementing this in #443, still have to write documentation.

ModProg added a commit to ModProg/derive_more that referenced this issue Feb 2, 2025
@ModProg
impl rename_all for #[derive(Display)] (JelteF#216)
3b4683f
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
enhancement help wanted
Projects
None yet
Milestone
2.1.0
Development

Successfully merging a pull request may close this issue.

impl rename_all for #[derive(Display)] (#216) ModProg/derive_more
4 participants
@JelteF @ModProg @ilslv @marcocondrache