-
-
Notifications
You must be signed in to change notification settings - Fork 4.3k
reflect PartialOrd #22452
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
base: main
Are you sure you want to change the base?
reflect PartialOrd #22452
Conversation
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.
thank you for the tests 🙏 I don’t see anything obviously wrong with what is written as someone who is new to reading bevy reflect code. Seems to follow the patterns of the other traits appropriately.
(No structural comparsion following derive(PartialOrd) is implemented though).
I wonder if this is worth making an issue for implementing if we go forward with merging this
| /// A custom implementation may be provided using `#[reflect(PartialEq(my_partial_eq_func))]` where | ||
| /// `my_partial_eq_func` is the path to a function matching the signature: | ||
| /// `(&Self, value: &dyn #bevy_reflect_path::Reflect) -> bool`. | ||
| /// * `#[reflect(PartialOrd)]` will let the implementation of `PartialReflect::reflect_partial_cmp` |
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.
If I’m understanding what you wrote correctly, and just to conform with what’s written for the other #[reflect(...)] comments:
| /// * `#[reflect(PartialOrd)]` will let the implementation of `PartialReflect::reflect_partial_cmp` | |
| /// * `#[reflect(PartialOrd)]` will force the implementation of `PartialReflect::reflect_partial_cmp` |
Objective
be able to compare ordering of two
dyn PartialReflectvaluesSolution.
add #[reflect(PartialOrd)] and method
PartialReflect::reflect_partial_cmp.(No structural comparsion following
derive(PartialOrd)is implemented though).Testing
Unit tests are added.