Implement Reflect for Result<T, E> as enum

[TheNeikos]

Objective

• Make Result<T, E> implement Reflect such that it is an Enum rather than a Value
• Fixes Result is reflected as a ReflectKind::Value, maybe could be an Enum instead? #13178

Solution

• Use the correct macro

Testing

• Did you test these changes?

I tried it out locally, and it does what it says on the tin. Not sure how to test it in context of the crate?

---

Changelog

Changed

• Result now uses ReflectKind::Enum rather than ReflectKind::Value, allowing for inspection of its constituents

Migration Guide

Result<T, E> has had its Reflect implementation changed to align it with Option<T> and its intended semantics: A carrier of either an Ok or Err value, and the ability to access it. To achieve this it is no longer a ReflectKind::Value but rather a ReflectKind::Enum and as such carries these changes with it:

For Result<T, E>

• Both T and E no longer require to be Clone and now require to be FromReflect
• <Result<T, E> as Reflect>::reflect_* now returns a ReflectKind::Enum, so any code that previously relied on it being a Value kind will have to be adapted.
• Result<T, E> now implements Enum

Since the migration is highly dependent on the previous usage, no automatic upgrade path can be given.

[github-actions]

It looks like your PR is a breaking change, but you didn't provide a migration guide.

Could you add some context on what users should update when this change get released in a new version of Bevy?
It will be used to help writing the migration guide for the version. Putting it after a ## Migration Guide will help it get automatically picked up by our tooling.

[soqb]

thanks for getting round to this!

LGTM: implementation is correct and in the right place, and i am hugely on board with the semantic improvement.

this is a subtle breaking change (beyond the acknowledged ReflectKind migration) since it changes the bounds on T and E. they here require FromReflect (IIRC) where they didn't previously and no longer imply Clone. ergo, it does needs a little note in the PR description.

[MrGVSV]

LGTM!

I agree that a Migration Guide should be added, though.

[TheNeikos]

@soqb @MrGVSV I added a Migration Guide to the PR Description. Not quite sure about the tone/addressee.

[MrGVSV]

@soqb @MrGVSV I added a Migration Guide to the PR Description. Not quite sure about the tone/addressee.

That works for me! Thanks for adding it!