Skip to content
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

[Merged by Bors] - bevy_reflect: Small refactor and default Reflect methods #4739

Closed
wants to merge 2 commits into from

Conversation

MrGVSV
Copy link
Member

@MrGVSV MrGVSV commented May 13, 2022

Objective

Quick followup to #4712.

While updating some other PRs, I realized the ReflectTraits struct could be improved. The issue with the current implementation is that ReflectTraits::get_xxx_impl(...) returns just the logic to the corresponding Reflect trait method, rather than the entire function.

This makes it slightly more annoying to manage since the variable names need to be consistent across files. For example, get_partial_eq_impl uses a value variable. But the name "value" isn't defined in the get_partial_eq_impl method, it's defined in three other methods in a completely separate file.

It's not likely to cause any bugs if we keep it as it is since differing variable names will probably just result in a compile error (except in very particular cases). But it would be useful to someone who wanted to edit/add/remove a method.

Solution

Made get_hash_impl, get_partial_eq_impl and get_serialize_impl return the entire method implementation for reflect_hash, reflect_partial_eq, and serializable, respectively.

As a result of this, those three Reflect methods were also given default implementations. This was fairly simple to do since all three could just be made to return None.


Changelog

  • Small cleanup/refactor to ReflectTraits in bevy_reflect_derive
  • Gave Reflect::reflect_hash, Reflect::reflect_partial_eq, and Reflect::serializable default implementations

Specifically added default implementations for reflect_hash,
reflect_partial_eq, and serializable
@alice-i-cecile 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 May 13, 2022
@alice-i-cecile
Copy link
Member

The big win here definitely feels like the default methods for Reflect :) Are we manually implementing None anywhere in our codebase for these?

@MrGVSV
Copy link
Member Author

MrGVSV commented May 13, 2022

The big win here definitely feels like the default methods for Reflect :)

Yeah maybe I should have focused more on that 😅

Are we manually implementing None anywhere in our codebase for these?

Yeah I found a handful of places that return None. Should I get rid of those? Got rid of them.

Remove Reflect trait methods that do exactly what their default
implementation does.
@alice-i-cecile
Copy link
Member

bors r+

@alice-i-cecile 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 May 18, 2022
bors bot pushed a commit that referenced this pull request May 18, 2022
# Objective

Quick followup to #4712.

While updating some [other PRs](#4218), I realized the `ReflectTraits` struct could be improved. The issue with the current implementation is that `ReflectTraits::get_xxx_impl(...)` returns just the _logic_ to the corresponding `Reflect` trait method, rather than the entire function.

This makes it slightly more annoying to manage since the variable names need to be consistent across files. For example, `get_partial_eq_impl` uses a `value` variable. But the name "value" isn't defined in the `get_partial_eq_impl` method, it's defined in three other methods in a completely separate file.

It's not likely to cause any bugs if we keep it as it is since differing variable names will probably just result in a compile error (except in very particular cases). But it would be useful to someone who wanted to edit/add/remove a method.

## Solution

Made `get_hash_impl`, `get_partial_eq_impl` and `get_serialize_impl` return the entire method implementation for `reflect_hash`, `reflect_partial_eq`, and `serializable`, respectively.

As a result of this, those three `Reflect` methods were also given default implementations. This was fairly simple to do since all three could just be made to return `None`.

---

## Changelog

* Small cleanup/refactor to `ReflectTraits` in `bevy_reflect_derive`
* Gave `Reflect::reflect_hash`, `Reflect::reflect_partial_eq`, and `Reflect::serializable` default implementations
@bors bors bot changed the title bevy_reflect: Small refactor and default Reflect methods [Merged by Bors] - bevy_reflect: Small refactor and default Reflect methods May 18, 2022
@bors bors bot closed this May 18, 2022
exjam pushed a commit to exjam/bevy that referenced this pull request May 22, 2022
…e#4739)

# Objective

Quick followup to bevyengine#4712.

While updating some [other PRs](bevyengine#4218), I realized the `ReflectTraits` struct could be improved. The issue with the current implementation is that `ReflectTraits::get_xxx_impl(...)` returns just the _logic_ to the corresponding `Reflect` trait method, rather than the entire function.

This makes it slightly more annoying to manage since the variable names need to be consistent across files. For example, `get_partial_eq_impl` uses a `value` variable. But the name "value" isn't defined in the `get_partial_eq_impl` method, it's defined in three other methods in a completely separate file.

It's not likely to cause any bugs if we keep it as it is since differing variable names will probably just result in a compile error (except in very particular cases). But it would be useful to someone who wanted to edit/add/remove a method.

## Solution

Made `get_hash_impl`, `get_partial_eq_impl` and `get_serialize_impl` return the entire method implementation for `reflect_hash`, `reflect_partial_eq`, and `serializable`, respectively.

As a result of this, those three `Reflect` methods were also given default implementations. This was fairly simple to do since all three could just be made to return `None`.

---

## Changelog

* Small cleanup/refactor to `ReflectTraits` in `bevy_reflect_derive`
* Gave `Reflect::reflect_hash`, `Reflect::reflect_partial_eq`, and `Reflect::serializable` default implementations
ItsDoot pushed a commit to ItsDoot/bevy that referenced this pull request Feb 1, 2023
…e#4739)

# Objective

Quick followup to bevyengine#4712.

While updating some [other PRs](bevyengine#4218), I realized the `ReflectTraits` struct could be improved. The issue with the current implementation is that `ReflectTraits::get_xxx_impl(...)` returns just the _logic_ to the corresponding `Reflect` trait method, rather than the entire function.

This makes it slightly more annoying to manage since the variable names need to be consistent across files. For example, `get_partial_eq_impl` uses a `value` variable. But the name "value" isn't defined in the `get_partial_eq_impl` method, it's defined in three other methods in a completely separate file.

It's not likely to cause any bugs if we keep it as it is since differing variable names will probably just result in a compile error (except in very particular cases). But it would be useful to someone who wanted to edit/add/remove a method.

## Solution

Made `get_hash_impl`, `get_partial_eq_impl` and `get_serialize_impl` return the entire method implementation for `reflect_hash`, `reflect_partial_eq`, and `serializable`, respectively.

As a result of this, those three `Reflect` methods were also given default implementations. This was fairly simple to do since all three could just be made to return `None`.

---

## Changelog

* Small cleanup/refactor to `ReflectTraits` in `bevy_reflect_derive`
* Gave `Reflect::reflect_hash`, `Reflect::reflect_partial_eq`, and `Reflect::serializable` default implementations
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Reflection Runtime information about types C-Code-Quality A section of code that is hard to understand or change S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants