Finish enhancing ReflectCommandExt to work with Bundles
#15152
Conversation
|
Welcome, new contributor! Please make sure you've read our contributing guide and we look forward to reviewing your pull request shortly ✨ |
alice-i-cecile
added
A-ECS
|
@miniex could I get your review on this follow-up work? :) |
| /// - If the component data is invalid. See [`PartialReflect::apply`] for further details. | ||
| /// - If [`AppTypeRegistry`] does not have the reflection data for the given | ||
| /// [`Component`](crate::component::Component) or [`Bundle`](crate::bundle::Bundle). | ||
| /// - If the component or bundle data is invalid. See [`PartialReflect::apply`] for further details. |
There was a problem hiding this comment.
In my opinion, it would be good to also write how to handle errors.
There was a problem hiding this comment.
Since this is a panic, would you not handle it like you would handle other bevy panics? Not familiar with how to handle panics myself, but if there's some documentation for it we can link it here
There was a problem hiding this comment.
Yeah, we don't have specific guidance here currently; I'm not sure there's more to add.
| /// commands.entity(prefab.entity) | ||
| /// .remove_reflect(prefab.component.reflect_type_path().to_owned()); | ||
| /// .remove_reflect(prefab.data.reflect_type_path().to_owned()); | ||
| /// } | ||
| /// | ||
| /// ``` |
There was a problem hiding this comment.
It would be nice to include safe removal methods and error handling examples.
There was a problem hiding this comment.
What do you mean by safe removal methods? Would this not be considered safe since it doesn't panic?
There was a problem hiding this comment.
My English is bad. I meant “safe” to mean preventing unexpected behavior, not preventing panic.
But based on the replies above, I don't think it's necessary. The overall feel of the finished code.
| /// commands.entity(prefab.entity) | ||
| /// .remove_reflect(prefab.component.reflect_type_path().to_owned()); | ||
| /// .remove_reflect(prefab.data.reflect_type_path().to_owned()); | ||
| /// } | ||
| /// | ||
| /// ``` |
There was a problem hiding this comment.
My English is bad. I meant “safe” to mean preventing unexpected behavior, not preventing panic.
But based on the replies above, I don't think it's necessary. The overall feel of the finished code.
alice-i-cecile
added
the
S-Ready-For-Final-Review
|
Awesome, thank you both. |
Objective
ReflectCommandExtwork with bundles #15125Solution
bevy_ecs::reflect::entity_commands::remove_reflectto handle both components and bundlesReflectCommandExtmethods to reflect that one can now use bundles with these commands.Testing