-
-
Notifications
You must be signed in to change notification settings - Fork 3.4k
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
reflection: replace impl_reflect_struct
with impl_reflect
#11437
reflection: replace impl_reflect_struct
with impl_reflect
#11437
Conversation
impl_reflect_struct
with impl_reflect
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code looks great, but this needs some tests validating that it actually works on tuple and enum types :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great!
#[derive(Debug, Copy, Clone, PartialEq, Eq)] | ||
pub(crate) struct ReflectProvenance { | ||
pub source: ReflectImplSource, | ||
pub trait_: ReflectTraitToImpl, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bikeshed: Rather than having to add the _
suffix, maybe we could rename this to trait_target
(maybe even renaming ReflectTraitToImpl
to ReflectTraitTarget
) or trait_to_impl
or something similar.
return Err(syn::Error::new( | ||
meta.type_path().span(), | ||
format!("a #[{TYPE_PATH_ATTRIBUTE_NAME} = \"...\"] attribute must be specified when using {provenance}") | ||
)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome!
crates/bevy_reflect/src/lib.rs
Outdated
@@ -1995,6 +1995,38 @@ bevy_reflect::tests::Test { | |||
); | |||
} | |||
|
|||
#[test] | |||
fn assert_impl_reflect_on_all() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you add assert_impl_all!
assertions for these types? Specifically to test that each receives their corresponding trait (i.e. Struct
for the struct, Enum
for the enum, etc.)? Maybe even to verify that they do indeed get all the other traits, like Typed
and friends.
I can't imagine we'd ever break something that big haha but could be nice to have a sanity check here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i added assert_impl_all
for Reflect
, which is sensible, but I don't see how we'd miss the others without also breaking #[derive(Reflect)]
.
Co-authored-by: Gino Valente <49806985+MrGVSV@users.noreply.github.com>
…gine#11437) # Objective - `impl_reflect_struct` doesn't cover tuple structs or enums. - Problem brought up [on Discord](https://discord.com/channels/691052431525675048/1002362493634629796/1190623345817960463). ## Solution - Replaces `impl_reflect_struct` with the new `impl_reflect` which works for tuple structs and enums too. --- ## Changelog - Internally in `bevy_reflect_derive`, we have a new `ReflectProvenance` type which is composed of `ReflectTraitToImpl` and `ReflectSource`. - `impl_reflect_struct` is gone and totally superseded by `impl_reflect`. --------- Co-authored-by: Gino Valente <49806985+MrGVSV@users.noreply.github.com>
Objective
impl_reflect_struct
doesn't cover tuple structs or enums.Solution
impl_reflect_struct
with the newimpl_reflect
which works for tuple structs and enums too.Changelog
bevy_reflect_derive
, we have a newReflectProvenance
type which is composed ofReflectTraitToImpl
andReflectSource
.impl_reflect_struct
is gone and totally superseded byimpl_reflect
.