-
-
Notifications
You must be signed in to change notification settings - Fork 4.4k
Description
What problem does this solve or what need does it fill?
Currently, all ***_partial_eq helper methods do something like:
bevy/crates/bevy_reflect/src/struct_trait.rs
Lines 455 to 457 in dfe9690
| if let Some(false) | None = field_value.reflect_partial_eq(value) { | |
| return Some(false); | |
| } |
If field_value.reflect_partial_eq(value) returned None, it makes more sense for the entire function to return None as this would suggest to the user that the comparison didn't just fail, but couldn't even be performed.
However, this is not currently done and instead the functions just return Some(false).
What solution would you like?
- Update the following code to return
Noneon a matchedNone:
bevy/crates/bevy_reflect/src/struct_trait.rs
Lines 455 to 457 in dfe9690
if let Some(false) | None = field_value.reflect_partial_eq(value) { return Some(false); } bevy/crates/bevy_reflect/src/tuple_struct.rs
Lines 366 to 368 in dfe9690
if let Some(false) | None = field_value.reflect_partial_eq(value) { return Some(false); } bevy/crates/bevy_reflect/src/tuple.rs
Line 386 in dfe9690
Some(false) | None => return Some(false), bevy/crates/bevy_reflect/src/array.rs
Lines 335 to 337 in dfe9690
if let Some(false) | None = a.reflect_partial_eq(b) { return Some(false); } bevy/crates/bevy_reflect/src/list.rs
Lines 309 to 311 in dfe9690
if let Some(false) | None = a_value.reflect_partial_eq(b_value) { return Some(false); } bevy/crates/bevy_reflect/src/map.rs
Lines 361 to 363 in dfe9690
if let Some(false) | None = value.reflect_partial_eq(map_value) { return Some(false); }
- Additionally, doc comments may need to be updated to reflect this change (or at least indicate why a
Nonevalue might be returned)
What alternative(s) have you considered?
Not doing this. Most usages of these functions and reflect_partial_eq itself use unwrap_or_default() anyways, so they would not be affected by such a change. However, adding this in allows us to give a bit more info to the users who want it.
Additional context
Originally brought to attention by @nicopap in #4761 (comment).