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

Update from_reflect to return a Result #7075

Open
wants to merge 22 commits into
base: main
Choose a base branch
from

Conversation

elbertronnie
Copy link
Contributor

@elbertronnie elbertronnie commented Jan 2, 2023

Objective

Solution

  • Create a new Enum ReflectKind which contains all the "kinds" of types in Reflect
  • Add a new method reflect_kind to Reflect which returns the appropriate kind
  • Create a new Error type FromReflectError
  • Update the function from_reflect to return a Result with the above Error
  • Replaced some panic calls with Result inside from_reflect

Changelog

  • Changed the return type of FromReflect::from_reflect to a Result with FromReflectError as the Error type
  • Created a new Enum ReflectKind which contains all the "kinds" of types in Reflect
  • Added a new method reflect_kind(&self) -> ReflectKind to Reflect which returns the appropriate kind

Migration Guide

  • Replace from_reflect(...) with from_reflect(...).ok()
  • If any of your types implement Reflect manually, add a new method reflect_kind(&self) -> ReflectKind to it

@james7132 james7132 added C-Feature A new feature, making something new possible A-Reflection Runtime information about types C-Usability A targeted quality-of-life change that makes Bevy easier to use labels Jan 2, 2023
@james7132 james7132 requested a review from MrGVSV January 2, 2023 18:04
@james7132 james7132 added the M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide label Jan 2, 2023
Copy link
Member

@MrGVSV MrGVSV left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good so far. I think the main thing that should be addressed (imo anyways) is making the error struct an enum instead.

crates/bevy_reflect/src/from_reflect.rs Outdated Show resolved Hide resolved
crates/bevy_reflect/src/from_reflect.rs Outdated Show resolved Hide resolved
crates/bevy_reflect/src/from_reflect.rs Outdated Show resolved Hide resolved
crates/bevy_reflect/src/from_reflect.rs Outdated Show resolved Hide resolved
crates/bevy_reflect/src/tuple.rs Outdated Show resolved Hide resolved
@elbertronnie
Copy link
Contributor Author

I have chosen the name ReflectKind since a lot of documentation already mentions them as "kinds".

To differentiate between Struct and DynamicStruct on source type, I have added a from_kind field to each of the errors. from_kind will determine if it is a Struct, Enum, List, etc and TypeInfo will determine if it is Dynamic or not.

The same cannot be done for the target type since calling reflect_kind(&self) requires self which is not available. We cannot even remove &self from reflect_kind since it will become an associated function making the Trait not object-safe. So I made an assumption that target type will most likely not be Dynamic and therefore used TypeInfo to determine the kind of target type.

Copy link
Member

@MrGVSV MrGVSV left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left a few more comments. Mostly documentation, organization, and other nitpicks.

I think we should also add some tests as well, to ensure that we get the proper errors from their corresponding cases. Could we add some to the from_reflect module? (We could continue to use lib.rs for this, but that file is already massive as is haha so it might be nicer to put them in their relevant module.)

crates/bevy_reflect/src/lib.rs Outdated Show resolved Hide resolved
crates/bevy_reflect/src/lib.rs Outdated Show resolved Hide resolved
crates/bevy_reflect/src/lib.rs Outdated Show resolved Hide resolved
crates/bevy_reflect/src/lib.rs Outdated Show resolved Hide resolved
crates/bevy_reflect/src/lib.rs Outdated Show resolved Hide resolved
crates/bevy_reflect/src/lib.rs Outdated Show resolved Hide resolved
crates/bevy_reflect/src/reflect.rs Outdated Show resolved Hide resolved
crates/bevy_reflect/src/reflect.rs Outdated Show resolved Hide resolved
crates/bevy_reflect/src/reflect.rs Outdated Show resolved Hide resolved
crates/bevy_reflect/src/reflect.rs Outdated Show resolved Hide resolved
@MrGVSV
Copy link
Member

MrGVSV commented Sep 29, 2024

@elbertronnie are you interested in rebasing this PR? If not, we can mark this as S-Adopt-Me (and I'll probably pick this up in that case)

@BenjaminBrienen BenjaminBrienen added D-Modest A "normal" level of difficulty; suitable for simple features or challenging fixes S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged D-Complex Quite challenging from either a design or technical perspective. Ask for help! and removed D-Modest A "normal" level of difficulty; suitable for simple features or challenging fixes labels Sep 29, 2024
@elbertronnie
Copy link
Contributor Author

@MrGVSV I am quite busy in other things currently. I can only work on this after about a month. If someone is willing to work on it now then I would suggest to mark it as S-Adopt-Me, otherwise I am willing to finish this incomplete PR after a month.

@elbertronnie
Copy link
Contributor Author

I found a blog a year back that had a good syntax for error messages related to reflection of composed structures. I think this could be useful to when implementing this PR. This is much better than the error messages that are currently in this PR.

Abductive diagnostics for Müsli

@MrGVSV
Copy link
Member

MrGVSV commented Sep 30, 2024

@MrGVSV I am quite busy in other things currently. I can only work on this after about a month. If someone is willing to work on it now then I would suggest to mark it as S-Adopt-Me, otherwise I am willing to finish this incomplete PR after a month.

I think we'll be okay to wait (unless you actually don't want to do it yourself, not trying to put any pressure on you haha). I’m planning to do some things with FromReflect soon, so if we decide we need it along with those changes then we can consider marking this for adoption.

@BenjaminBrienen
Copy link
Contributor

Wouldn't TryFromReflect be a better name now?

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-Feature A new feature, making something new possible C-Usability A targeted quality-of-life change that makes Bevy easier to use D-Complex Quite challenging from either a design or technical perspective. Ask for help! M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Enable error returning from FromReflect trait
4 participants