Skip to content

Conversation

@LikeLakers2
Copy link
Contributor

Objective

Solution

Set the clippy::allow_attributes and clippy::allow_attributes_without_reason lints to deny, and bring bevy_ui in line with the new restrictions.

Testing

cargo clippy --tests and cargo test --package bevy_ui were run, and no errors were encountered.

@LikeLakers2 LikeLakers2 force-pushed the lint/deny_allow_and_without_reason/bevy_ui branch from 425d98e to 95d4ba2 Compare January 8, 2025 09:38
@LikeLakers2 LikeLakers2 force-pushed the lint/deny_allow_and_without_reason/bevy_ui branch from 95d4ba2 to 33e311b Compare January 8, 2025 09:39
Comment on lines 466 to 469
#[allow(unreachable_code)]
#[expect(
unreachable_code,
reason = "Certain pieces of code tested here cause the test to fail if made reachable; see #17231 for progress on fixing this"
)]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am not proficient enough in bevy_ui to know if we should merge these expects that I added to crates/bevy_ui/src/layout/ui_surface.rs, or if we should wait until #17231 is fixed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was informed that there's already a PR for this: #16362

@mnmaita mnmaita added C-Code-Quality A section of code that is hard to understand or change D-Straightforward Simple bug fixes and API improvements, docs, test and examples S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Jan 8, 2025
Copy link
Contributor

@StrikeForceZero StrikeForceZero left a comment

Choose a reason for hiding this comment

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

in case you missed it from the last CI run

error: #[allow] attribute found
 --> crates/bevy_ui/src/render/debug_overlay.rs:57:3
  |
57 | #[allow(clippy::too_many_arguments)]
  |   ^^^^^ help: replace it with: `expect`
error: `allow` attribute without specifying a reason
  --> crates/bevy_ui/src/render/debug_overlay.rs:57:1
   |
57 | #[allow(clippy::too_many_arguments)]
   | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
   |
   = help: try adding a reason at the end with `, reason = ".."`

Excluding my other comment and the above, everything looks good to me.

@LikeLakers2
Copy link
Contributor Author

@StrikeForceZero I indeed missed it - because I intentionally ignored CI, thinking the errors were going to be about the #[expect(unreachable_code)]s.

I'll get it later today, as I can't check whether those need to be removed or replaced with expect()s without being at my dev computer.

@LikeLakers2 LikeLakers2 marked this pull request as ready for review January 8, 2025 19:14
@LikeLakers2
Copy link
Contributor Author

Marked as ready for review per @alice-i-cecile - because whether my my PR is merged first, or @StrikeForceZero's PR is merged first, the other will have a minor merge conflict.

@alice-i-cecile alice-i-cecile added S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it and removed S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Jan 8, 2025
@alice-i-cecile alice-i-cecile added this pull request to the merge queue Jan 8, 2025
Merged via the queue into bevyengine:main with commit 8b4c25a Jan 8, 2025
29 checks passed
@LikeLakers2 LikeLakers2 deleted the lint/deny_allow_and_without_reason/bevy_ui branch January 8, 2025 19:50
mrchantey pushed a commit to mrchantey/bevy that referenced this pull request Feb 4, 2025
…butes_without_reason)]` (bevyengine#17229)

# Objective
- bevyengine#17111

## Solution
Set the `clippy::allow_attributes` and
`clippy::allow_attributes_without_reason` lints to `deny`, and bring
`bevy_ui` in line with the new restrictions.

## Testing
`cargo clippy --tests` and `cargo test --package bevy_ui` were run, and
no errors were encountered.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

C-Code-Quality A section of code that is hard to understand or change D-Straightforward Simple bug fixes and API improvements, docs, test and examples 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.

4 participants