-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Improve needless_lifetimes
#9743
Conversation
r? @Alexendoo (rust-highfive has picked a reviewer for you, use r? to override) |
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.
Nice! Just a few small things/questions
Catching those <'a, 'tcx>
ones is great
Hey, could you maybe expand the changelog entry a bit? It fine if it spans over multiple lines for complex changes. You're welcome to reach out if you want some assistance :) |
To be completely honest, I have never completely understood what should go in those entries.
|
In Clippy, we create the changelog based on these comments. Every release, someone gets all commits from bors with a
Sadly, I have no direct example, the entries of the changelog are a good level. Interested users open the PR if they really want additional information. The rule of thumb is basically what do you believe is important for an outsider perspective. Often, PRs are only related to a single property of the lint, and then it's good to list that one. Otherwise, it's better to include too much detail than too little. I sometimes look at the changelog and then adapt an existing entry to my change.
There is no fixed format, also because it would be more work to enforce than actually beneficial. The main point is, that the writer should clearly see that the lines belong together. I've seen and liked this: changelog: Something 1
changelog: Something 2
changelog: Something 3 We could be a bit clearer on the creation of the changelog. I hope this answers your questions, reach out if I missed a point, or you would like more context :) |
@xFrednet Please tell me if what I have now suffices. |
I'm unsure which false negative you mean with the first entry:
The others are excellent, thank you very much for the update. |
Here's some added cases it lints to skim through, looks good to me
|
Ugh. Sorry, @xFrednet. It should be better now:
|
Thanks very much for this, @Alexendoo. I changed those two |
Yep, looks all good to me thanks! @bors r+ |
☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test |
Update CONTRIBUTING.md with changelog guidance `@xFrednet` I cribbed some text of yours from #9743 (comment) and did some light editing in the hope that others might find it. r? `@xFrednet` changelog: Update CONTRIBUTING.md with changelog guidance
Use `walk_generic_arg` This is a tiny followup to to #9743, now that rust-lang/rust#103692 has landed. r? `@Alexendoo` changelog: none
This PR makes the following improvements to
needless_lifetimes
.foo
is flagged butbar
is not:needless_borrow
required all lifetimes to be used only once. With the changes, individual lifetimes are flagged for being used only once, even if not all lifetimes are.changelog: fix
needless_lifetimes
false negative involving functions with multiple unnamed lifetimeschangelog: in
needless_lifetimes
, flag individual lifetimes used only once, rather than require all lifetimes to be used only oncechangelog: in
needless_lifetimes
, emit "replace with'_
" warnings only when applicable, and point to a generic argument