Skip to content

Lint against the use of unwrap #20914

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

Jump to bottom
Open
wants to merge 21 commits into
base: main
Choose a base branch
from
Open

Lint against the use of unwrap #20914

wants to merge 21 commits into from

Conversation

james7132
Copy link
Member

Objective

Begin addressing #12660.

Solution

Follow the example of the existing efforts to enable deny(missing_docs) and deny(unsafe_code):

  • Set clippy::unwrap_used to warn workspace wide.
  • Set allow-unwrap-in-tests to true in Clippy.toml.
  • Add expect annotations for every non-trivial crate in the workspace
  • Directly address a few crates with only a few cases.

Testing

Ran clippy locally.

@james7132 james7132 added C-Code-Quality A section of code that is hard to understand or change A-Cross-Cutting Impacts the entire engine D-Straightforward Simple bug fixes and API improvements, docs, test and examples S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Sep 7, 2025
@alice-i-cecile
Copy link
Member

Very happy with the approach and core idea here. Ping me when CI is green for a review?

@alice-i-cecile alice-i-cecile added S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged and removed S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Sep 7, 2025
BD103
BD103 reviewed Sep 7, 2025
View reviewed changes
@james7132 james7132 added the M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide label Sep 8, 2025
@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Sep 8, 2025

It looks like your PR is a breaking change, but you didn't provide a migration guide.

Please review the instructions for writing migration guides, then expand or revise the content in the migration guides directory to reflect your changes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
1 more reviewer

@BD103 BD103 BD103 left review comments

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
A-Cross-Cutting Impacts the entire engine 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 M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@james7132 @alice-i-cecile @BD103