-
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
Add type details to unnecessary_fallible_conversions note #11767
Add type details to unnecessary_fallible_conversions note #11767
Conversation
r? @blyxyas (rustbot has picked a reviewer for you, use r? to override) |
f9b6361
to
a826d78
Compare
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.
Just a couple nits and this should be ready!
with_forced_trimmed_paths!({ | ||
diag.note(format!("converting `{source_ty}` to `{target_ty}` which cannot fail")); | ||
|
||
diag.span_suggestion(span, "use", sugg, applicability); | ||
}); |
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.
with_forced_trimmed_paths
should be closed after the note, it isn't necessary on the suggestion
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.
Done
a826d78
to
1bd73d7
Compare
applicability, | ||
|diag| { | ||
with_forced_trimmed_paths!({ | ||
diag.note(format!("converting `{source_ty}` to `{target_ty}` which cannot fail")); |
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.
Hmmm I'm not sure about this phrasing 🤔
Maybe removing that "which"? What do you think about it
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.
I have no strong preference for the phrasing of the note.
A version without the "which" is maybe a bit clearer/ direct, so if you prefer that version then I can happily change it.
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.
Okis, I think removing the "which" is the better route here. It should be merge-ready from here on.
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.
Done
1bd73d7
to
4dead77
Compare
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.
LGTM, thanks! ❤️
@bors r+ |
…otes, r=blyxyas Add type details to unnecessary_fallible_conversions note fixes: #11753 changelog: [`unnecessary_fallible_conversions`]: add type details to lint note
☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test |
? It didn't push? |
@bors r+ |
💡 This pull request was already approved, no need to approve it again. |
☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test |
fixes: #11753
changelog: [
unnecessary_fallible_conversions
]: add type details to lint note