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.

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

Make restriction lint's use span_lint_and_then (i -> p) #13144

Merged
merged 3 commits into from
Jul 26, 2024

Conversation

xFrednet
Copy link
Member

This migrates a few restriction lints to use span_lint_and_then. This change is motivated by #7797.

I've also cleaned up some lint message. Mostly minor stuff. For example: suggestions with a longer message than "try" now use SuggestionStyle::ShowAlways


cc: #7797

brother PR of: #13136

changelog: none

@rustbot
Copy link
Collaborator

rustbot commented Jul 22, 2024

r? @y21

rustbot has assigned @y21.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Jul 22, 2024
@y21
Copy link
Member

y21 commented Jul 23, 2024

Is the plan to completely move away from these other span_lint_and_* functions? I feel like we should just remove that internal lint then 🤔. Seems odd that we require using functions that shouldn't be used or that we have to add #[expect]s everywhere.

I really liked span_lint_and_{sugg,help} more for when the messages are just plain string literals and that seems like it shouldn't make a difference there, but oh well

@xFrednet
Copy link
Member Author

Is the plan to completely move away from these other span_lint_and_* functions?

That's a good question. The issue suggests the entire removal of these functions to have one unified way. I also think that rustc's diagnostic documentation is pretty good, which would make it easier for new contributors.

For restriction lints, I think it makes highly sense to remove them. Lints like implicit_return had previously created a suggestion for every implicit return. I slightly changed the message, and that's where lintcheck got the 60000+ changes from.

Now, I've kept the internal lint around, as I'm unsure, how motivated I am to migrate all of Clippy. These three PRs represent ~116 of our 700+ lints. Even if we create a migration lint it would take dedication. And rn I'm not sure if I'm committed enough for that. If we don't migrate all of Clippy, it would be better to keep the lint for consistency.

Copy link
Member

@y21 y21 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, just two small "formatting" nits.

clippy_lints/src/methods/map_err_ignore.rs Outdated Show resolved Hide resolved
clippy_lints/src/float_literal.rs Outdated Show resolved Hide resolved
@xFrednet xFrednet force-pushed the 07797-restriction-and-then-what branch from f2c96f5 to 5cf3893 Compare July 25, 2024 07:26
@xFrednet
Copy link
Member Author

Very good catches! Thank you for taking the time =^.^=

The last commit, should have resolved everything.

@y21
Copy link
Member

y21 commented Jul 25, 2024

Looks all good to me now! Want to squash the last commit or keep it? You can r=me either way

@xFrednet xFrednet force-pushed the 07797-restriction-and-then-what branch from 5cf3893 to 90c1963 Compare July 26, 2024 07:41
@xFrednet
Copy link
Member Author

And squashed :D

Thank you for the quick feedback =^.^=


Roses are red,
Violets are blue,
This has been approved,
by the one and only Timo

@bors
Copy link
Contributor

bors commented Jul 26, 2024

📌 Commit 90c1963 has been approved by y21

It is now in the queue for this repository.

@bors
Copy link
Contributor

bors commented Jul 26, 2024

⌛ Testing commit 90c1963 with merge 345c94c...

@bors
Copy link
Contributor

bors commented Jul 26, 2024

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: y21
Pushing 345c94c to master...

@bors bors merged commit 345c94c into rust-lang:master Jul 26, 2024
8 checks passed
@xFrednet xFrednet deleted the 07797-restriction-and-then-what branch July 26, 2024 08:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants