Peternator7 / strum

A small rust library for adding custom derives to enums
https://crates.io/crates/strum
MIT License
1.8k stars 152 forks source link

fix!: use the tuple value for to_string on default #270

Closed joshka closed 1 year ago

joshka commented 1 year ago

The default attribute on a tuple like variant now causes the to_string and display format to use the value of the tuple rather than the name of the variant.

E.g. Color::Green("lime").to_string() will equal "lime" not "Green"

Fixes: how to round trip Display and EnumString with default="true" https://github.com/Peternator7/strum/issues/86

BREAKING CHANGE This changes how Display and ToString cause the following to renders:

#[strum(default)]
Green(String)

To maintain the previous behavior (use the variant name):

#[strum(default, to_string("Green"))]
Green(String)
joshka commented 1 year ago

This is the no breaking changes approach. It could reasonably break changes by just making this the default behavior. Happy to see this as a PoC of the functionality, and ask for feedback on how it should be configured.

Some options:

// the current implementation in this PR
#[strum(default, default_to_string)]

// perhaps this PR's behavior should just be the default
#[strum(default)]

// perhaps we could pass a to_string with no value (or with "")
#[strum(default, to_string)]

Perhaps the default to_string for a default variant should automatically include the data item e.g. Green("lime-green") or Green(lime-green), with attributes controlling whether to use only the data or to rename the variant (e.g. SomeOtherVariantName(dataPassedToTheVariant))

I don't really have a good enough feel for what the correct approach should be on this, so happy to take any guidance.

joshka commented 1 year ago

I'm not 100% sure why the 1.32 breakage on this happens :?

jirutka commented 1 year ago

I was unpleasantly surprised that default is not symmetric. This feature is very much needed!

joshka commented 1 year ago

I was unpleasantly surprised that default is not symmetric. This feature is very much needed!

Same - it's not a big deal, but it meant having to work around it for that catchall case. Do you have any thoughts about the ergonomics of the API of this feature? I haven't used strum before and I'm fairly new to rust, so don't know what would is idiomatic here (and it seems like this crate has some pretty high standards for backwards compatible changes, so getting it right is a one time thing).

joshka commented 1 year ago

Incidentally running cargo msrv on a mac suggests that the MSRV right now is 1.54.0, not 1.32.0.

Peternator7 commented 1 year ago

Hey @joshka, I've been thinking about this, and I think you're right. 0.25 is going to have a couple of breaking changes so this seems like a good time to include this one as well. Let's make this the default behavior, and if you can, add an attribute that brings back the old behavior if that's what someone really wants. Seem reasonable?

joshka commented 1 year ago

Sounds like a good plan. Just to confirm, when there is no to_string attribute, we render the tuple

#[strum(default]
Custom(String)
// Custom("lime") should render as "lime"

while if there is a to_string attribute, we just ignore the tuple:

#[strum(default, to_string("custom color")]
Custom(String)
// Custom("lime") should render as "custom color"

and at least for now there's no way to render something like "custom color: lime"

joshka commented 1 year ago

Updated PR title and body in case you use github to push rather than doing it locally.

New commit message:


fix!: use the tuple value for to_string on default

The default attribute on a tuple like variant now causes the to_string and display format to use the value of the tuple rather than the name of the variant.

E.g. Color::Green("lime").to_string() will equal "lime" not "Green"

Fixes: how to round trip Display and EnumString with default="true" https://github.com/Peternator7/strum/issues/86

BREAKING CHANGE This changes how Display and ToString cause the following to renders:

#[strum(default)]
Green(String)

To maintain the previous behavior (use the variant name):

#[strum(default, to_string("Green"))]
Green(String)
joshka commented 1 year ago

I'm not sure why this is failing with 1.32. Any thoughts?

Peternator7 commented 1 year ago

Just release 0.25.0 which has this upgrade! It's packaged with a couple of other breaking changes. Appreciate the work on this. I think it'll be much more consistent for users.