-
-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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
Conversation
There was a problem hiding this 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
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)); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Works for me!
bors r+ |
# 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.
Pull request successfully merged into main. Build succeeded: |
…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.
…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.
…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.
…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.
…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.
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.