search

illicitonion / num_enum

Apache License 2.0
264 stars 34 forks source link

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

Open AlexSherbinin opened 8 months ago

AlexSherbinin commented 8 months ago

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.

illicitonion commented 5 months ago

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.