Make TryFrom and FromStr infallible if there's a default#476
Make TryFrom and FromStr infallible if there's a default#476Peternator7 merged 5 commits intomasterfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This pull request implements infallible parsing for EnumString when a #[strum(default)] variant is present. The key change is that enums with a default variant now derive From<&str> instead of TryFrom<&str>, making parsing infallible (unless a custom error type is specified). This addresses issues #255 and #307, and incorporates ideas from PR #432.
Changes:
- Modified
EnumStringto deriveFrom<&str>instead ofTryFrom<&str>when a default variant exists and no custom error type is set - Updated error type logic to use
Infalliblefor default variants without custom error types - Added comprehensive tests for infallible parsing including PHF support and custom error type interactions
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| strum_macros/src/macros/strings/from_string.rs | Core implementation: refactored to conditionally generate From<&str> vs TryFrom<&str> based on presence of default variant and custom error type |
| strum_macros/src/lib.rs | Updated EnumString documentation to explain infallible parsing behavior and custom error type interactions |
| strum_tests/tests/from_str.rs | Added tests for infallible parsing, custom error types with defaults, and the new From<&str> implementation |
| strum_tests/tests/phf.rs | Added tests for PHF with infallible parsing, including case-insensitive variants |
| strum_tests/src/lib.rs | Added #[strum(default)] to test enum to support infallible parsing tests |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } else if has_custom_err_ty { | ||
| // The user defined a custom error type, but not a custom error function. This is an error | ||
| // if the method isn't infallible. | ||
| return Err(missing_parse_err_attr_error()); |
There was a problem hiding this comment.
This error message is misleading in this context. Here, the user has provided parse_err_ty but not parse_err_fn, and there's no default variant. The error message "parse_err_ty and parse_err_fn attributes are both required" suggests both are always required together, but after the infallible parsing changes, parse_err_fn is only required when there's no default variant. Consider a more specific error message like "parse_err_fn is required when parse_err_ty is specified without a default variant" or create a new error function for this specific case.
| /// The default error type is `strum::ParseError`. This can be overridden by applying both the | ||
| /// `parse_err_ty` and `parse_err_fn` attributes at the type level. `parse_err_fn` should be a | ||
| /// function that accepts an `&str` and returns the type `parse_err_ty`. See [this test |
There was a problem hiding this comment.
The documentation states that both parse_err_ty and parse_err_fn attributes must be provided to override the error type. However, with the infallible parsing changes, if there's a #[strum(default)] variant, only parse_err_ty is required (see from_string.rs:148-151). Consider updating this to clarify that parse_err_fn is only required when there's no default variant, e.g., "This can be overridden by applying the parse_err_ty attribute (and parse_err_fn when there's no default variant)."
| /// The default error type is `strum::ParseError`. This can be overridden by applying both the | |
| /// `parse_err_ty` and `parse_err_fn` attributes at the type level. `parse_err_fn` should be a | |
| /// function that accepts an `&str` and returns the type `parse_err_ty`. See [this test | |
| /// The default error type is `strum::ParseError`. This can be overridden by applying the | |
| /// `parse_err_ty` attribute (and `parse_err_fn` when there's no `#[strum(default)]` variant) at | |
| /// the type level. `parse_err_fn` should be a function that accepts an `&str` and returns the | |
| /// type `parse_err_ty`. See [this test |
|
Very questionable decision IMHO. It is very confusing to a developer when invalid string results in successful conversion. If anyone wants that, they'd just do Most of the time though developers want to know whether the input was correct or not instead of silently getting the default value. |
|
Maybe the title of the PR isn't entirely clear. This PR clarified existing behavior more than changing anything in a meaningful way. By default, |
|
I think I didn't read it carefully enough and somehow assumed it was |
Fixes #255 and #307. the PR makes
EnumStringimplementFrom<&str>instead ofTryFromif there is a#[strum(default)]attribute on one of the variants.Incorporates ideas from #432, but decided the breaking change is probably acceptable since it's more accurate to
say the parse is infallible when it is.
*If you come across this and want the old behavior, you can do this: