-
-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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.
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
Apply clippy::use_self #14798
Apply clippy::use_self #14798
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Way nicer for refactoring names. There are some cases where I wouldn't use Self
, but the pros outweigh the cons imo
I'm in favor of this for its consistency, but I haven't looked through each file yet. |
Personally I think it's not only nicer for refactoring. It's also quite nice when reading code as its instantly clear something is exactly the given type. |
I don't think we should do this. This is a noisy change, and more importantly, I prefer the other style. When reviewing / reading code snippets, it's much harder to constantly keep track of what scope we're in, rather than just using the names. This has been attempted before in #4152 and was rejected by maintainers there. Closing this out as well, sorry. |
When going through the changes I noticed a bunch of places where this is already used.
This PR was about ~1600 lines touched, there are already 7k with Self. So this change is not that significant in the regard of using To quote the consensus of #4152:
Guess I should have searched the existing PR about |
Yeah, I prefer the other style, but without a lint to enforce it I don't think it's worth spending time to update the other style. |
Objective
When looking through the changes #14782 would produce I noticed another lint (
clippy::use_self
) that should be quite neat here (and solves some ofunused_qualifications
cases already which is why I don't want to supply a PR for both at the same time.)In a bunch of cases
Self
is already used near the suggestion to useSelf
ofuse_self
. So I assume this is accepted code which this lint enforces further in the future.Solution
Add
clippy::use_self
(and alphabetically sort its lints) and mostly--fix
its way through the code.cargo fmt
results in less lines for the same code afterwards. Also removed some empty lines here and there.I also manually fixed the false-negative rust-lang/rust-clippy#12381 in the cases I noticed.
--fix
didn't work inbevy_ecs
which turned out to be a clippy false-positive: rust-lang/rust-clippy#13277Testing
cargo clippy
still seems to be happy.cargo run -p ci
seems to work too. (It produces some errors of this kind, but it doesn't seem relevant for this PR:Could not set global logger and tracing subscriber as they are already set. Consider disabling LogPlugin.
)Reviewing this is probably quite messy as this changes hundreds of files. This will likely also produce many merge conflicts. I am happy to integrate other merged branches for some time if interest in merging this is there. But similar to #10749 it shouldn't stay open for too long due to its amount of affected files.