-
-
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] - Document remaining members of bevy_utils #6897
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.
Just one little suggestion
Co-authored-by: ira <JustTheCoolDude@gmail.com>
/// This method tests for `self` and `other` values to be equal. | ||
/// | ||
/// Implementors should avoid returning `true` when the underlying types are | ||
/// not the same. |
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 think maybe you can be a little stronger with the language here. I don't see a reason at all for someone to return true if the underlying types are different. Though maybe I am missing a use case for something like that?
Maybe just replace avoid
with not
?
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.
That's an interesting one. Should 0u64.dyn_eq(0u32)
return true? IMO it probably should.
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.
Should this trait be manually implementable at all? Adding the bound DynEq: Eq
should make any impls other than the blanket impl impossible.
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.
I'd rather not introduce a breaking change in a docs PR, and even then it may not be desirable force it to only work with same type to same type equality.
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.
Thats a good point!
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
Co-authored-by: Carter Weinberg <weinbergcarter@gmail.com>
bors r+ |
# Objective Partially address #3492. ## Solution Document the remaining undocumented members of `bevy_utils` and set `warn(missing_docs)` on the crate level. Also enabled `clippy::undocumented_unsafe_blocks` as a warning on the crate to keep it in sync with `bevy_ecs`'s warnings.
Pull request successfully merged into main. Build succeeded:
|
# Objective Partially address bevyengine#3492. ## Solution Document the remaining undocumented members of `bevy_utils` and set `warn(missing_docs)` on the crate level. Also enabled `clippy::undocumented_unsafe_blocks` as a warning on the crate to keep it in sync with `bevy_ecs`'s warnings.
# Objective Partially address bevyengine#3492. ## Solution Document the remaining undocumented members of `bevy_utils` and set `warn(missing_docs)` on the crate level. Also enabled `clippy::undocumented_unsafe_blocks` as a warning on the crate to keep it in sync with `bevy_ecs`'s warnings.
Objective
Partially address #3492.
Solution
Document the remaining undocumented members of
bevy_utils
and setwarn(missing_docs)
on the crate level. Also enabledclippy::undocumented_unsafe_blocks
as a warning on the crate to keep it in sync withbevy_ecs
's warnings.