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

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

Merged
merged 1 commit into from Jun 18, 2023

Conversation

joshka
Copy link
Contributor

@joshka joshka commented May 16, 2023

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" #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
Copy link
Contributor Author

joshka commented May 16, 2023

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
Copy link
Contributor Author

joshka commented May 16, 2023

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

@jirutka
Copy link

jirutka commented May 17, 2023

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

@joshka
Copy link
Contributor Author

joshka commented May 17, 2023

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
Copy link
Contributor Author

joshka commented May 22, 2023

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

@Peternator7
Copy link
Owner

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
Copy link
Contributor Author

joshka commented May 26, 2023

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"

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" Peternator7#86

BREAKING CHANGE
This changes how Display and ToString cause the following to renders:
```rust
#[strum(default)]
Green(String)
```
To maintain the previous behavior (use the variant name):
```rust
#[strum(default, to_string("Green"))]
Green(String)
```
@joshka joshka changed the title add default_to_string attribute fix!: use the tuple value for to_string on default May 27, 2023
@joshka
Copy link
Contributor Author

joshka commented May 27, 2023

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" #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
Copy link
Contributor Author

joshka commented Jun 9, 2023

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

@Peternator7 Peternator7 merged commit e6a9bf0 into Peternator7:master Jun 18, 2023
1 check failed
@joshka joshka deleted the joshka/issue86 branch June 18, 2023 05:18
@Peternator7
Copy link
Owner

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.

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

Successfully merging this pull request may close these issues.

None yet

3 participants