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

Enable error returning from FromReflect trait #5967

Open
afonsolage opened this issue Sep 12, 2022 · 3 comments · May be fixed by #7075
Open

Enable error returning from FromReflect trait #5967

afonsolage opened this issue Sep 12, 2022 · 3 comments · May be fixed by #7075
Labels
A-Reflection Runtime information about types C-Usability A targeted quality-of-life change that makes Bevy easier to use D-Trivial Nice and easy! A great choice to get started with Bevy

Comments

@afonsolage
Copy link
Contributor

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 afonsolage added C-Feature A new feature, making something new possible S-Needs-Triage This issue needs to be labelled labels Sep 12, 2022
@alice-i-cecile alice-i-cecile added C-Usability A targeted quality-of-life change that makes Bevy easier to use A-Reflection Runtime information about types and removed C-Feature A new feature, making something new possible S-Needs-Triage This issue needs to be labelled labels Sep 13, 2022
@alice-i-cecile
Copy link
Member

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

@MrGVSV MrGVSV added the D-Trivial Nice and easy! A great choice to get started with Bevy label Nov 22, 2022
@zeroacez
Copy link
Contributor

zeroacez commented Dec 13, 2022

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?

@alice-i-cecile
Copy link
Member

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.

@elbertronnie elbertronnie linked a pull request Jan 2, 2023 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Reflection Runtime information about types C-Usability A targeted quality-of-life change that makes Bevy easier to use D-Trivial Nice and easy! A great choice to get started with Bevy
Projects
Status: Open
Development

Successfully merging a pull request may close this issue.

4 participants