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] - Implemented Reflect for all the ranges #5806

Closed
wants to merge 1 commit into from

Conversation

maccesch
Copy link
Contributor

@maccesch maccesch commented Aug 26, 2022

Objective

Fixes #5763

Solution

Implemented as reflect value like the current Range. Is there a benefit to changing everything to a reflect struct?

@alice-i-cecile alice-i-cecile added C-Usability A targeted quality-of-life change that makes Bevy easier to use A-Reflection Runtime information about types labels Aug 27, 2022
@Weibye Weibye 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 Aug 27, 2022
@@ -41,6 +41,11 @@ impl_reflect_value!(String(Debug, Hash, PartialEq, Serialize, Deserialize));
impl_reflect_value!(Result<T: Clone + Reflect + 'static, E: Clone + Reflect + 'static>());
impl_reflect_value!(HashSet<T: Hash + Eq + Clone + Send + Sync + 'static>());
impl_reflect_value!(Range<T: Clone + Send + Sync + 'static>());
impl_reflect_value!(RangeInclusive<T: Clone + Send + Sync + 'static>());
Copy link
Member

@MrGVSV MrGVSV Aug 29, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: Not that you have to, since none of the others do this, but you could replace some of those bounds with Reflect itself:

Suggested change
impl_reflect_value!(RangeInclusive<T: Clone + Send + Sync + 'static>());
impl_reflect_value!(RangeInclusive<T: Clone + Reflect>());

Feel free to resolve this if you'd rather keep it as is haha


Edit: I guess this suggestion would require that the inner type is reflectable as well, which might be a good reason to hold off on applying it.

Comment on lines 43 to +47
impl_reflect_value!(Range<T: Clone + Send + Sync + 'static>());
impl_reflect_value!(RangeInclusive<T: Clone + Send + Sync + 'static>());
impl_reflect_value!(RangeFrom<T: Clone + Send + Sync + 'static>());
impl_reflect_value!(RangeTo<T: Clone + Send + Sync + 'static>());
impl_reflect_value!(RangeToInclusive<T: Clone + Send + Sync + 'static>());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: You have an extra space before each Send (likely due to Range having one too).

Suggested change
impl_reflect_value!(Range<T: Clone + Send + Sync + 'static>());
impl_reflect_value!(RangeInclusive<T: Clone + Send + Sync + 'static>());
impl_reflect_value!(RangeFrom<T: Clone + Send + Sync + 'static>());
impl_reflect_value!(RangeTo<T: Clone + Send + Sync + 'static>());
impl_reflect_value!(RangeToInclusive<T: Clone + Send + Sync + 'static>());
impl_reflect_value!(Range<T: Clone + Send + Sync + 'static>());
impl_reflect_value!(RangeInclusive<T: Clone + Send + Sync + 'static>());
impl_reflect_value!(RangeFrom<T: Clone + Send + Sync + 'static>());
impl_reflect_value!(RangeTo<T: Clone + Send + Sync + 'static>());
impl_reflect_value!(RangeToInclusive<T: Clone + Send + Sync + 'static>());

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd make these changes myself, but I've been disallowed from pushing to this branch :)
Worth blocking on as we'll just forget about it if we merge now.

@cart
Copy link
Member

cart commented Aug 30, 2022

Eh actually I'll just follow up. No sense waiting around

@cart
Copy link
Member

cart commented Aug 30, 2022

bors r+

bors bot pushed a commit that referenced this pull request Aug 30, 2022
# Objective

Fixes #5763

## Solution

Implemented as reflect value like the current `Range`. Is there a benefit to changing everything to a reflect struct?
@bors bors bot changed the title Implemented Reflect for all the ranges [Merged by Bors] - Implemented Reflect for all the ranges Aug 30, 2022
@bors bors bot closed this Aug 30, 2022
james7132 pushed a commit to james7132/bevy that referenced this pull request Oct 28, 2022
# Objective

Fixes bevyengine#5763

## Solution

Implemented as reflect value like the current `Range`. Is there a benefit to changing everything to a reflect struct?
ItsDoot pushed a commit to ItsDoot/bevy that referenced this pull request Feb 1, 2023
# Objective

Fixes bevyengine#5763

## Solution

Implemented as reflect value like the current `Range`. Is there a benefit to changing everything to a reflect struct?
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-Usability A targeted quality-of-life change that makes Bevy easier to use 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.

Implement Reflect for the remaining Range types
6 participants