Skip to content
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

Allow clippy::too_many_arguments to lint without warnings #17249

Merged
merged 3 commits into from
Jan 9, 2025
Merged

Allow clippy::too_many_arguments to lint without warnings #17249

merged 3 commits into from
Jan 9, 2025

Conversation

LikeLakers2
Copy link
Contributor

Objective

Many instances of clippy::too_many_arguments linting happen to be on systems - functions which we don't call manually, and thus there's not much reason to worry about the argument count.

Solution

Allow clippy::too_many_arguments globally, and remove all lint attributes related to it.

@BenjaminBrienen BenjaminBrienen 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 Jan 9, 2025
rparrett
rparrett approved these changes Jan 9, 2025
View reviewed changes
Copy link
Contributor

@rparrett rparrett left a comment

Choose a reason for hiding this comment

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

Looks good to me.

@BenjaminBrienen BenjaminBrienen 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 9, 2025
@LikeLakers2
Copy link
Contributor Author

LikeLakers2 commented Jan 9, 2025 •
edited
Loading

I keep feeling like I went way too fast in submitting this PR. As a result, I'm not actually sure if this is what we want, or if we want to do something slightly different.

So I'm switching this to draft mode for now, until I can figure out what we want.

@LikeLakers2 LikeLakers2 marked this pull request as draft January 9, 2025 02:32
@alice-i-cecile alice-i-cecile added the X-Blessed Has a large architectural impact or tradeoffs, but the design has been endorsed by decision makers label Jan 9, 2025
@alice-i-cecile
Copy link
Member

I think its easiest for the bevy community as a whole if we just encourage globally disabling that lint as a matter of course

I agree with this position too.

@LikeLakers2
Copy link
Contributor Author

Well, if two maintainers (one of them being the project lead) think that, then I think I'm going to un-draft this PR.

@LikeLakers2 LikeLakers2 marked this pull request as ready for review January 9, 2025 02:35
@alice-i-cecile alice-i-cecile added this pull request to the merge queue Jan 9, 2025
@LikeLakers2 LikeLakers2 mentioned this pull request Jan 9, 2025
Closed
Merged via the queue into bevyengine:main with commit 3742e62 Jan 9, 2025
32 checks passed
@LikeLakers2 LikeLakers2 deleted the lint/allow_too_many_arguments branch January 9, 2025 07:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@BenjaminBrienen BenjaminBrienen BenjaminBrienen approved these changes

@rparrett rparrett rparrett approved these changes

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 S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it X-Blessed Has a large architectural impact or tradeoffs, but the design has been endorsed by decision makers
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@LikeLakers2 @alice-i-cecile @rparrett @BenjaminBrienen