add ReflectFromTemplate and ReflectTemplate#24930
Conversation
|
Welcome, new contributor! Please make sure you've read our contributing guide, as well as our policy regarding AI usage, and we look forward to reviewing your pull request shortly ✨ |
MrGVSV
left a comment
There was a problem hiding this comment.
Looking good! I left some minor nitpicks. Can we also add docs and format the code please?
| } | ||
| } | ||
|
|
||
| impl<T: Reflect + Template<Output=impl Reflect> + TypePath> CreateTypeData<T> for ReflectTemplate { |
There was a problem hiding this comment.
I honestly had no clue you could do impl Reflect here. How does this differ from writing Template<Output: Reflect>?
There was a problem hiding this comment.
I checked and, writing <Output: Reflect> doesn't work, it needs to be either dyn Reflect or impl Reflect
| Self(fns) | ||
| } | ||
|
|
||
| pub fn fn_pointer(&self) -> &ReflectTemplateFns { |
There was a problem hiding this comment.
Nit: We should probably update the name to be plural
| pub fn fn_pointer(&self) -> &ReflectTemplateFns { | |
| pub fn fn_pointers(&self) -> &ReflectTemplateFns { |
|
|
||
| fn clone_template(&self) -> Self { | ||
| Self { | ||
| foo: self.foo.clone() |
There was a problem hiding this comment.
Nit: I don't think the call to clone is needed here
| foo: self.foo.clone() | |
| foo: self.foo |
| #[cfg(feature = "reflect_auto_register")] | ||
| registry.register_derived_types(); | ||
| #[cfg(not(feature = "reflect_auto_register"))] | ||
| { | ||
| registry.register::<MyStruct>(); | ||
| registry.register_type_data::<MyStruct, ReflectFromTemplate>(); | ||
| registry.register::<MyStructTemplate>(); | ||
| registry.register_type_data::<MyStructTemplate, ReflectTemplate>(); | ||
| registry.register_type_data::<MyStructTemplate, ReflectDefault>(); | ||
| } |
There was a problem hiding this comment.
Personally, I think we should just manually register types in tests. It's simpler, more explicit, and doesn't rely on automatic-registration working properly (in the case reflect_auto_register is enabled).
| #[cfg(feature = "reflect_auto_register")] | |
| registry.register_derived_types(); | |
| #[cfg(not(feature = "reflect_auto_register"))] | |
| { | |
| registry.register::<MyStruct>(); | |
| registry.register_type_data::<MyStruct, ReflectFromTemplate>(); | |
| registry.register::<MyStructTemplate>(); | |
| registry.register_type_data::<MyStructTemplate, ReflectTemplate>(); | |
| registry.register_type_data::<MyStructTemplate, ReflectDefault>(); | |
| } | |
| registry.register::<MyStruct>(); | |
| registry.register_type_data::<MyStruct, ReflectFromTemplate>(); | |
| registry.register::<MyStructTemplate>(); | |
| registry.register_type_data::<MyStructTemplate, ReflectTemplate>(); | |
| registry.register_type_data::<MyStructTemplate, ReflectDefault>(); |
| ); | ||
| let my_struct_reflect = reflect_template.build( | ||
| &mut template_context, | ||
| template.as_partial_reflect(), |
There was a problem hiding this comment.
Nit: I think we should also be able to automatically upcast like &*template (i.e. dereference the Box). We're working to deprecate these methods (see #21772).
| let my_struct = my_struct_reflect.downcast::<MyStruct>().expect("Should be MyStruct"); | ||
|
|
||
| assert_eq!( | ||
| *my_struct.deref(), |
There was a problem hiding this comment.
Nit: The * should be enough to dereference the Box
| *my_struct.deref(), | |
| *my_struct, |
| Self(fns) | ||
| } | ||
|
|
||
| pub fn fn_pointer(&self) -> &ReflectFromTemplateFns { |
There was a problem hiding this comment.
Nit: Same here, can we make this plural?
| pub fn fn_pointer(&self) -> &ReflectFromTemplateFns { | |
| pub fn fn_pointers(&self) -> &ReflectFromTemplateFns { |
Objective
implementation for issue (#24870)
Solution
Add ReflectFromTemplate and ReflectTemplate for creating Templates and their Output from reflection.
Derive Reflect for all Templates and Reflect FromTemplate and Template.
Testing
Added test for fetching the associated ReflectTemplate for a reflected struct and creating the struct.
Showcase