bevy icon indicating copy to clipboard operation
bevy copied to clipboard

Enable error returning from `FromReflect` trait

Open afonsolage opened this issue 2 years ago • 1 comments

What problem does this solve or what need does it fill?

When dealing with FromReflect implementation other than derive, it's hard to know where the error is, since if you have a big tree of nested dyn Reflect objects, any of them may fail and return None.

What solution would you like?

One of those:

  1. Add a new method try_from_reflect which does the actual parsing and change from_reflect to a default impl which just call try_from_reflect and convert Result into Option;
  2. Modify from_reflect to return a Result instead of Option;

I prefer 1, since it doesn't introduce a braking change for those which already uses from_reflect, but will break for any manual FromReflect impl, which I think will be fewer use cases.

What alternative(s) have you considered?

Relying on panic or error messages for any manual FromReflect impl

Additional context

If there is a consensus about this feature, I can impl it

afonsolage avatar Sep 12 '22 19:09 afonsolage

My preference is 2: I think this clearer and more idiomatic. I don't mind the breaking change.

alice-i-cecile avatar Sep 13 '22 16:09 alice-i-cecile

Hi! Would it be acceptable to replace Option with an anyhow::Result or is a standard Result the way to go? In the case of a standard Result, what would the preferred error type be?

zeroacez avatar Dec 13 '22 14:12 zeroacez

Standard result with a custom error type describing the possible failure modes :) I'm not sure one exists yet.

Anyhow's results should not be used in library code as they don't contain enough information on how something failed to be automatically actionable.

alice-i-cecile avatar Dec 13 '22 14:12 alice-i-cecile