num_enum icon indicating copy to clipboard operation
num_enum copied to clipboard

TryFromPrimitive should return an error on `default` and `catch_all`

Open AlexSherbinin opened this issue 11 months ago • 1 comments

Now when using #[num_enum(default)] or #[num_enum(default)] with TryFromPrimitive it just ignores them. But it's more reasonable to return error because of missing support.

AlexSherbinin avatar Mar 10 '24 08:03 AlexSherbinin

This is a really interesting question to consider, thanks for raising it!

My initial instinct is that it's unusual to derive both FromPrimitive (which supports default) and TryFromPrimitive (which doesn't) - in general, either an enum always has a reasonable default (where FromPrimitive makes sense), or the default is context-specific (and FromPrimitive doesn't have the context of how/where it's being used, so TryFromPrimitive makes sense, and the caller should probably do something like try_into().unwrap_or_default(..)).

Which suggests that we should probably error if num_enum(default) is present and TryFromPrimitive is being derived.

That said, this would be a breaking change, as currently it is possible to derive both. It probably makes sense to make that change though.

Out of interest did this cause you a real problem when using the library, or did you just notice the inconsistency and raise it? Both are valid reasons to fix this, but if you ran into a problem as a result of this issue I'd lean much more heavily towards fixing this soon.

illicitonion avatar Jun 08 '24 14:06 illicitonion