bevy_color: Apply #![deny(clippy::allow_attributes, clippy::allow_attributes_without_reason)]
#17090
Conversation
…ttributes_without_reason)]` to `bevy_color`
IQuick143
added
C-Code-Quality
crates/bevy_color/src/oklaba.rs
Outdated
| #[allow(clippy::excessive_precision)] | ||
| #[expect( | ||
| clippy::excessive_precision, | ||
| reason = "The code with excessive precision was copied from another codebase." | ||
| )] |
There was a problem hiding this comment.
(Forgot to copy this comment over.)
I'm not really happy with these two lint attributes (this one, and the one shown in the comment below), as there really isn't a reason for the floats to have excessive precision, beyond the code having been copied verbatim from another code-base.
That said, I feel truncating the floats is out-of-scope for this PR, hence me changing the lint attributes to be expect(...).
There was a problem hiding this comment.
I'd say the rationale could be to be consistent with other implementations?
But I agree that truncating the floats is out of scope here, also maybe for documentation purposes (If someone were to use our code as a template and worked with a greater precision. Alternatively you're expressing the intent of multiplying with these precise values, even if it's not going to be achievable with f32's.)
There was a problem hiding this comment.
Hmm. I'm not so sure about that rationale, personally. I don't really know how to explain why, but it just doesn't sit right with me.
I mean, I'm willing to change it to that - reason = "Excessive precision is used here to be consistent with other implementations." - but I'd maybe like a second opinion before I do that.
There was a problem hiding this comment.
I like the reason in your last message better!
There was a problem hiding this comment.
The original source of these numbers is actually https://bottosson.github.io/posts/oklab/#converting-from-linear-srgb-to-oklab.
There was a problem hiding this comment.
So should I change the reason to this?
reason = "Excessive precision is used here to be consistent with other implementations."Or are we still discussing what it should be?
There was a problem hiding this comment.
I would just truncate. The extra digits don't have a meaning. (Or you could say that the numbers match the reference implementation)
There was a problem hiding this comment.
@eero-lehtinen I would truncate - but honestly, doing so feels like it'd be out-of-scope for this PR.
There was a problem hiding this comment.
That reason you suggested or what I said is completely fine. Just ship it.
There was a problem hiding this comment.
Truncated floats are visible at this PR: #17109
I'm not a fan of the truncated floats, as they reduce readability... I would personally just do as I have in this PR, and move the allow to an expect.
But I'll see what Alice/other maintainers say.
crates/bevy_color/src/oklaba.rs
Outdated
| #[allow(clippy::excessive_precision)] | ||
| #[expect( | ||
| clippy::excessive_precision, | ||
| reason = "The code with excessive precision was copied from another codebase." | ||
| )] |
alice-i-cecile
added
S-Ready-For-Final-Review
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
|
@alice-i-cecile Requesting this be re-added to the merge queue. |
LikeLakers2
changed the title
#[deny(clippy::allow_attributes, clippy::allow_attributes_without_reason)]#![deny(clippy::allow_attributes, clippy::allow_attributes_without_reason)]
Objective
clippy::allow_attributesandclippy::allow_attributes_without_reasonlints #17111Solution
Set the
clippy::allow_attributesandclippy::allow_attributes_without_reasonlints todeny, and bringbevy_audioin line with the new restrictions.No code changes have been made - except if a lint that was previously
allow(...)'d could be removed via small code changes. For example,unused_variablescan be handled by adding a_to the beginning of a field's name.Testing
I ran
cargo clippy, and received no errors.