Skip to content

bevy_color: Apply #![deny(clippy::allow_attributes, clippy::allow_attributes_without_reason)] #17090

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.

Sign up for GitHub

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
merged 4 commits into from
Jan 5, 2025
Merged

bevy_color: Apply #![deny(clippy::allow_attributes, clippy::allow_attributes_without_reason)] #17090

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
910437a
Apply `#[deny(clippy::allow_attributes)]` and `#[deny(clippy::allow_a…
LikeLakers2 Jan 2, 2025
13745b6
Please the almighty `rustfmt`
LikeLakers2 Jan 2, 2025
df74dee
Update the deny to link to the tracking issue
LikeLakers2 Jan 3, 2025
1ff4c5a
Merge branch 'main' into lint/deny_allow_and_without_reason/bevy_color
LikeLakers2 Jan 3, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 9 additions & 1 deletion crates/bevy_color/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
#![cfg_attr(docsrs, feature(doc_auto_cfg))]
#![forbid(unsafe_code)]
#![deny(clippy::allow_attributes, clippy::allow_attributes_without_reason)]
#![doc(
html_logo_url = "https://bevyengine.org/assets/icon.png",
html_favicon_url = "https://bevyengine.org/assets/icon.png"
Expand Down Expand Up @@ -142,7 +143,14 @@ pub use srgba::*;
pub use xyza::*;

/// Describes the traits that a color should implement for consistency.
#[allow(dead_code)] // This is an internal marker trait used to ensure that our color types impl the required traits
#[expect(
clippy::allow_attributes,
reason = "If the below attribute on `dead_code` is removed, then rustc complains that `StandardColor` is dead code. However, if we `expect` the `dead_code` lint, then rustc complains of an unfulfilled expectation."
)]
#[allow(
dead_code,
reason = "This is an internal marker trait used to ensure that our color types impl the required traits"
)]
pub(crate) trait StandardColor
where
Self: core::fmt::Debug,
Expand Down
10 changes: 8 additions & 2 deletions crates/bevy_color/src/oklaba.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -216,7 +216,10 @@ impl ColorToComponents for Oklaba {
}
}

#[allow(clippy::excessive_precision)]
#[expect(
clippy::excessive_precision,
reason = "The code with excessive precision was copied from another codebase."
)]
Copy link
Contributor Author

@LikeLakers2 LikeLakers2 Jan 2, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

(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(...).

Copy link
Contributor

Choose a reason for hiding this comment

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

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.)

Copy link
Contributor Author

@LikeLakers2 LikeLakers2 Jan 2, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

I like the reason in your last message better!

Copy link
Contributor

@eero-lehtinen eero-lehtinen Jan 2, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

The original source of these numbers is actually https://bottosson.github.io/posts/oklab/#converting-from-linear-srgb-to-oklab.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

@eero-lehtinen eero-lehtinen Jan 2, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I would just truncate. The extra digits don't have a meaning. (Or you could say that the numbers match the reference implementation)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@eero-lehtinen I would truncate - but honestly, doing so feels like it'd be out-of-scope for this PR.

Copy link
Contributor

@eero-lehtinen eero-lehtinen Jan 3, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

That reason you suggested or what I said is completely fine. Just ship it.

Copy link
Contributor Author

@LikeLakers2 LikeLakers2 Jan 3, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

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.

impl From<LinearRgba> for Oklaba {
fn from(value: LinearRgba) -> Self {
let LinearRgba {
Expand All @@ -239,7 +242,10 @@ impl From<LinearRgba> for Oklaba {
}
}

#[allow(clippy::excessive_precision)]
#[expect(
clippy::excessive_precision,
reason = "The code with excessive precision was copied from another codebase."
)]
Copy link
Contributor Author

@LikeLakers2 LikeLakers2 Jan 2, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Ditto.

impl From<Oklaba> for LinearRgba {
fn from(value: Oklaba) -> Self {
let Oklaba {
Expand Down
1 change: 0 additions & 1 deletion crates/bevy_color/src/palettes/css.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@
use crate::Srgba;

// The CSS4 colors are a superset of the CSS1 colors, so we can just re-export the CSS1 colors.
#[allow(unused_imports)]
pub use crate::palettes::basic::*;

/// <div style="background-color:rgb(94.1%, 97.3%, 100.0%); width: 10px; padding: 10px; border: 1px solid;"></div>
Expand Down
Loading