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] - Added reflect/from reflect impls for NonZero integer types #5556

Closed
wants to merge 3 commits into from

Conversation

maxwellodri
Copy link
Contributor

@maxwellodri maxwellodri commented Aug 3, 2022

Objective

Add reflect/from reflect impls for NonZero integer types. I'm guessing these haven't been added yet because no one has needed them as of yet.

Copy link
Member

@MrGVSV MrGVSV left a comment

Choose a reason for hiding this comment

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

LGTM! One comment but not blocking

Comment on lines +46 to +57
impl_reflect_value!(NonZeroI128(Debug, Hash, PartialEq, Serialize, Deserialize));
impl_reflect_value!(NonZeroU128(Debug, Hash, PartialEq, Serialize, Deserialize));
impl_reflect_value!(NonZeroIsize(Debug, Hash, PartialEq, Serialize, Deserialize));
impl_reflect_value!(NonZeroUsize(Debug, Hash, PartialEq, Serialize, Deserialize));
impl_reflect_value!(NonZeroI64(Debug, Hash, PartialEq, Serialize, Deserialize));
impl_reflect_value!(NonZeroU64(Debug, Hash, PartialEq, Serialize, Deserialize));
impl_reflect_value!(NonZeroU32(Debug, Hash, PartialEq, Serialize, Deserialize));
impl_reflect_value!(NonZeroI32(Debug, Hash, PartialEq, Serialize, Deserialize));
impl_reflect_value!(NonZeroI16(Debug, Hash, PartialEq, Serialize, Deserialize));
impl_reflect_value!(NonZeroU16(Debug, Hash, PartialEq, Serialize, Deserialize));
impl_reflect_value!(NonZeroU8(Debug, Hash, PartialEq, Serialize, Deserialize));
impl_reflect_value!(NonZeroI8(Debug, Hash, PartialEq, Serialize, Deserialize));
Copy link
Member

Choose a reason for hiding this comment

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

Nit: I’m sure this works and all, but could we add a test for at least one of these types? Ideally we'd test both Reflect and FromReflect are working.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ive added a single test (one assert for reflect, one for from reflect) - I can add more if needed however.

Copy link
Member

Choose a reason for hiding this comment

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

Works for me!

@alice-i-cecile alice-i-cecile added A-Reflection Runtime information about types C-Usability A simple 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 labels Aug 3, 2022
@cart
Copy link
Member

cart commented Aug 4, 2022

bors r+

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

Add reflect/from reflect impls for NonZero integer types. I'm guessing these haven't been added yet because no one has needed them as of yet.
@bors bors bot changed the title Added reflect/from reflect impls for NonZero integer types [Merged by Bors] - Added reflect/from reflect impls for NonZero integer types Aug 4, 2022
@bors bors bot closed this Aug 4, 2022
maccesch pushed a commit to Synphonyte/bevy that referenced this pull request Aug 5, 2022
…e#5556)

# Objective

Add reflect/from reflect impls for NonZero integer types. I'm guessing these haven't been added yet because no one has needed them as of yet.
inodentry pushed a commit to IyesGames/bevy that referenced this pull request Aug 8, 2022
…e#5556)

# Objective

Add reflect/from reflect impls for NonZero integer types. I'm guessing these haven't been added yet because no one has needed them as of yet.
inodentry pushed a commit to BroovyJammy/bevy that referenced this pull request Aug 19, 2022
…e#5556)

# Objective

Add reflect/from reflect impls for NonZero integer types. I'm guessing these haven't been added yet because no one has needed them as of yet.
james7132 pushed a commit to james7132/bevy that referenced this pull request Oct 28, 2022
…e#5556)

# Objective

Add reflect/from reflect impls for NonZero integer types. I'm guessing these haven't been added yet because no one has needed them as of yet.
ItsDoot pushed a commit to ItsDoot/bevy that referenced this pull request Feb 1, 2023
…e#5556)

# Objective

Add reflect/from reflect impls for NonZero integer types. I'm guessing these haven't been added yet because no one has needed them as of yet.
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 simple 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.

None yet

5 participants