[Merged by Bors] - Make reflect_partial_eq return more accurate results #5210

afonsolage · 2022-07-05T09:59:04Z

Objective

Closes #5204

Solution

Followed @nicopap suggestion on [Merged by Bors] - bevy_reflect: Reflect enums #4761 (comment)

Changelog

Updated struct_trait, tuple_struct, tuple, array, list and map to return None when comparison couldn't be performed.
Updated docs comments to reflect above changes.

nicopap

LGTM, but maybe this could wait until #4761 is merged?

alice-i-cecile · 2022-07-05T11:29:18Z

crates/bevy_reflect/src/array.rs

-                if let Some(false) | None = a.reflect_partial_eq(b) {
-                    return Some(false);
+                let eq_result = a.reflect_partial_eq(b);
+                if let failed @ (Some(false) | None) = eq_result {


Woah, what's this @ syntax called?

https://doc.rust-lang.org/book/ch18-03-pattern-syntax.html#-bindings

The reference doesn't mention the name. It just specifies it as a subset of identifier patterns. The rust book call them "@ bindings".

reference: https://doc.rust-lang.org/reference/patterns.html#identifier-patterns

rust book: https://doc.rust-lang.org/book/ch18-03-pattern-syntax.html#-bindings

In traditional functional languages (Haskell, Ocaml), they usually use the as keyword. In rust this wouldn't work since it's the explicit casting operator. Looks like the Ocaml reference calls them "alias patterns", which I think is pretty descriptive https://v2.ocaml.org/manual/patterns.html#sss:pat-alias

That was new to me too. I had to check docs when I saw the @(at) pattern bindings. I'm glad I learn something new and useful.

MrGVSV

LGTM!

MrGVSV · 2022-07-05T15:05:43Z

crates/bevy_reflect/src/tuple.rs

        match a_field.reflect_partial_eq(b_field) {
-            Some(false) | None => return Some(false),
+            failed @ (Some(false) | None) => return failed,
            Some(true) => {}
        }


Nit: For consistency with the others, maybe this should be an if statement?

I had the same thought, but I didn't wanted to touch to make changes as simple as possible. But this doesn't seems to be enough change to have it's own PR, so I can do it without any problems.

alice-i-cecile · 2022-07-05T15:59:24Z

@afonsolage can you add a brief Changelog section to your PR description to capture the changed behavior? You can just borrow from the linked issue.

Breaking change label added as this is subtly different (and more correct) behavior that will need migration work.

afonsolage · 2022-07-05T17:06:03Z

@afonsolage can you add a brief Changelog section to your PR description to capture the changed behavior? You can just borrow from the linked issue.

Breaking change label added as this is subtly different (and more correct) behavior that will need migration work.

Added changelog. This way is good?

alice-i-cecile · 2022-07-05T17:12:25Z

Looks great. I probably wouldn't note the doc comment changes; those should accompany every change.

alice-i-cecile · 2022-07-05T17:13:50Z

@afonsolage once you address the nit raised above I'll merge this in :)

alice-i-cecile · 2022-07-05T17:41:39Z

bors r+

@nicopap

# Objective Closes #5204 ## Solution - Followed @nicopap suggestion on #4761 (comment) ## Changelog - [x] Updated [struct_trait](https://github.com/bevyengine/bevy/blob/dfe969005264fff54060f9fb148639f80f9cfb29/crates/bevy_reflect/src/struct_trait.rs#L455-L457), [tuple_struct](https://github.com/bevyengine/bevy/blob/dfe969005264fff54060f9fb148639f80f9cfb29/crates/bevy_reflect/src/tuple_struct.rs#L366-L368), [tuple](https://github.com/bevyengine/bevy/blob/dfe969005264fff54060f9fb148639f80f9cfb29/crates/bevy_reflect/src/tuple.rs#L386), [array](https://github.com/bevyengine/bevy/blob/dfe969005264fff54060f9fb148639f80f9cfb29/crates/bevy_reflect/src/array.rs#L335-L337), [list](https://github.com/bevyengine/bevy/blob/dfe969005264fff54060f9fb148639f80f9cfb29/crates/bevy_reflect/src/list.rs#L309-L311) and [map](https://github.com/bevyengine/bevy/blob/dfe969005264fff54060f9fb148639f80f9cfb29/crates/bevy_reflect/src/map.rs#L361-L363) to return `None` when comparison couldn't be performed. - [x] Updated docs comments to reflect above changes.

bors · 2022-07-05T17:54:31Z

Pull request successfully merged into main.

Build succeeded:

@nicopap

# Objective Closes bevyengine#5204 ## Solution - Followed @nicopap suggestion on bevyengine#4761 (comment) ## Changelog - [x] Updated [struct_trait](https://github.com/bevyengine/bevy/blob/dfe969005264fff54060f9fb148639f80f9cfb29/crates/bevy_reflect/src/struct_trait.rs#L455-L457), [tuple_struct](https://github.com/bevyengine/bevy/blob/dfe969005264fff54060f9fb148639f80f9cfb29/crates/bevy_reflect/src/tuple_struct.rs#L366-L368), [tuple](https://github.com/bevyengine/bevy/blob/dfe969005264fff54060f9fb148639f80f9cfb29/crates/bevy_reflect/src/tuple.rs#L386), [array](https://github.com/bevyengine/bevy/blob/dfe969005264fff54060f9fb148639f80f9cfb29/crates/bevy_reflect/src/array.rs#L335-L337), [list](https://github.com/bevyengine/bevy/blob/dfe969005264fff54060f9fb148639f80f9cfb29/crates/bevy_reflect/src/list.rs#L309-L311) and [map](https://github.com/bevyengine/bevy/blob/dfe969005264fff54060f9fb148639f80f9cfb29/crates/bevy_reflect/src/map.rs#L361-L363) to return `None` when comparison couldn't be performed. - [x] Updated docs comments to reflect above changes.

@nicopap

# Objective Closes bevyengine#5204 ## Solution - Followed @nicopap suggestion on bevyengine#4761 (comment) ## Changelog - [x] Updated [struct_trait](https://github.com/bevyengine/bevy/blob/dfe969005264fff54060f9fb148639f80f9cfb29/crates/bevy_reflect/src/struct_trait.rs#L455-L457), [tuple_struct](https://github.com/bevyengine/bevy/blob/dfe969005264fff54060f9fb148639f80f9cfb29/crates/bevy_reflect/src/tuple_struct.rs#L366-L368), [tuple](https://github.com/bevyengine/bevy/blob/dfe969005264fff54060f9fb148639f80f9cfb29/crates/bevy_reflect/src/tuple.rs#L386), [array](https://github.com/bevyengine/bevy/blob/dfe969005264fff54060f9fb148639f80f9cfb29/crates/bevy_reflect/src/array.rs#L335-L337), [list](https://github.com/bevyengine/bevy/blob/dfe969005264fff54060f9fb148639f80f9cfb29/crates/bevy_reflect/src/list.rs#L309-L311) and [map](https://github.com/bevyengine/bevy/blob/dfe969005264fff54060f9fb148639f80f9cfb29/crates/bevy_reflect/src/map.rs#L361-L363) to return `None` when comparison couldn't be performed. - [x] Updated docs comments to reflect above changes.

@nicopap

# Objective Closes bevyengine#5204 ## Solution - Followed @nicopap suggestion on bevyengine#4761 (comment) ## Changelog - [x] Updated [struct_trait](https://github.com/bevyengine/bevy/blob/dfe969005264fff54060f9fb148639f80f9cfb29/crates/bevy_reflect/src/struct_trait.rs#L455-L457), [tuple_struct](https://github.com/bevyengine/bevy/blob/dfe969005264fff54060f9fb148639f80f9cfb29/crates/bevy_reflect/src/tuple_struct.rs#L366-L368), [tuple](https://github.com/bevyengine/bevy/blob/dfe969005264fff54060f9fb148639f80f9cfb29/crates/bevy_reflect/src/tuple.rs#L386), [array](https://github.com/bevyengine/bevy/blob/dfe969005264fff54060f9fb148639f80f9cfb29/crates/bevy_reflect/src/array.rs#L335-L337), [list](https://github.com/bevyengine/bevy/blob/dfe969005264fff54060f9fb148639f80f9cfb29/crates/bevy_reflect/src/list.rs#L309-L311) and [map](https://github.com/bevyengine/bevy/blob/dfe969005264fff54060f9fb148639f80f9cfb29/crates/bevy_reflect/src/map.rs#L361-L363) to return `None` when comparison couldn't be performed. - [x] Updated docs comments to reflect above changes.

afonsolage added 8 commits July 5, 2022 06:39

Updated struct_trait

1dcbb94

Updated tuple_struct

facf9ed

Updated tuple

77ca886

Updated array

c52483f

Updated list

7819c8c

Updated map

383f3cd

Updated doc comments to reflect new changes

93f49a1

Fixed fmt

fc67513

nicopap reviewed Jul 5, 2022

View reviewed changes

alice-i-cecile reviewed Jul 5, 2022

View reviewed changes

alice-i-cecile added C-Code-Quality A section of code that is hard to understand or change A-Reflection Runtime information about types labels Jul 5, 2022

MrGVSV approved these changes Jul 5, 2022

View reviewed changes

alice-i-cecile approved these changes Jul 5, 2022

View reviewed changes

alice-i-cecile added the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label Jul 5, 2022

alice-i-cecile added the M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide label Jul 5, 2022

converted match to if statement for consistency

61333ca

bors bot changed the title ~~Make reflect_partial_eq return more accurate results~~ [Merged by Bors] - Make reflect_partial_eq return more accurate results Jul 5, 2022

bors bot closed this Jul 5, 2022

rparrett mentioned this pull request Jul 23, 2022

[Merged by Bors] - 0.8 Migration guide bevyengine/bevy-website#389

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Merged by Bors] - Make reflect_partial_eq return more accurate results #5210

[Merged by Bors] - Make reflect_partial_eq return more accurate results #5210

afonsolage commented Jul 5, 2022 •

edited

Loading

nicopap left a comment

alice-i-cecile Jul 5, 2022

mockersf Jul 5, 2022

nicopap Jul 5, 2022 •

edited

Loading

afonsolage Jul 5, 2022

MrGVSV left a comment

MrGVSV Jul 5, 2022

afonsolage Jul 5, 2022

alice-i-cecile commented Jul 5, 2022 •

edited

Loading

afonsolage commented Jul 5, 2022

alice-i-cecile commented Jul 5, 2022 •

edited

Loading

alice-i-cecile commented Jul 5, 2022

alice-i-cecile commented Jul 5, 2022

bors bot commented Jul 5, 2022

[Merged by Bors] - Make reflect_partial_eq return more accurate results #5210

[Merged by Bors] - Make reflect_partial_eq return more accurate results #5210

Conversation

afonsolage commented Jul 5, 2022 • edited Loading

Objective

Solution

Changelog

nicopap left a comment

Choose a reason for hiding this comment

alice-i-cecile Jul 5, 2022

Choose a reason for hiding this comment

mockersf Jul 5, 2022

Choose a reason for hiding this comment

nicopap Jul 5, 2022 • edited Loading

Choose a reason for hiding this comment

afonsolage Jul 5, 2022

Choose a reason for hiding this comment

MrGVSV left a comment

Choose a reason for hiding this comment

MrGVSV Jul 5, 2022

Choose a reason for hiding this comment

afonsolage Jul 5, 2022

Choose a reason for hiding this comment

alice-i-cecile commented Jul 5, 2022 • edited Loading

afonsolage commented Jul 5, 2022

alice-i-cecile commented Jul 5, 2022 • edited Loading

alice-i-cecile commented Jul 5, 2022

alice-i-cecile commented Jul 5, 2022

bors bot commented Jul 5, 2022

afonsolage commented Jul 5, 2022 •

edited

Loading

nicopap Jul 5, 2022 •

edited

Loading

alice-i-cecile commented Jul 5, 2022 •

edited

Loading

alice-i-cecile commented Jul 5, 2022 •

edited

Loading