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

Add type details to unnecessary_fallible_conversions note #11767

Merged

Conversation

matthri
Copy link
Contributor

@matthri matthri commented Nov 7, 2023

fixes: #11753

changelog: [unnecessary_fallible_conversions]: add type details to lint note

@rustbot
Copy link
Collaborator

rustbot commented Nov 7, 2023

r? @blyxyas

(rustbot has picked a reviewer for you, use r? to override)

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Nov 7, 2023
@matthri matthri force-pushed the unnecessary-fallible-conversions-ext-notes branch from f9b6361 to a826d78 Compare November 7, 2023 15:14
Copy link
Member

@blyxyas blyxyas left a 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!

Comment on lines 97 to 99
with_forced_trimmed_paths!({
diag.note(format!("converting `{source_ty}` to `{target_ty}` which cannot fail"));

diag.span_suggestion(span, "use", sugg, applicability);
});
Copy link
Member

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@matthri matthri force-pushed the unnecessary-fallible-conversions-ext-notes branch from a826d78 to 1bd73d7 Compare November 11, 2023 19:23
applicability,
|diag| {
with_forced_trimmed_paths!({
diag.note(format!("converting `{source_ty}` to `{target_ty}` which cannot fail"));
Copy link
Member

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

Copy link
Contributor Author

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.

Copy link
Member

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@matthri matthri force-pushed the unnecessary-fallible-conversions-ext-notes branch from 1bd73d7 to 4dead77 Compare November 11, 2023 20:01
Copy link
Member

@blyxyas blyxyas left a comment

Choose a reason for hiding this comment

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

LGTM, thanks! ❤️

@blyxyas
Copy link
Member

blyxyas commented Nov 11, 2023

@bors r+

@bors
Copy link
Collaborator

bors commented Nov 11, 2023

📌 Commit 4dead77 has been approved by blyxyas

It is now in the queue for this repository.

@bors
Copy link
Collaborator

bors commented Nov 11, 2023

⌛ Testing commit 4dead77 with merge 6148447...

bors added a commit that referenced this pull request Nov 11, 2023
…otes, r=blyxyas

Add type details to unnecessary_fallible_conversions note

fixes: #11753

changelog: [`unnecessary_fallible_conversions`]: add type details to lint note
@bors
Copy link
Collaborator

bors commented Nov 11, 2023

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: blyxyas
Pushing 6148447 to master...

@blyxyas
Copy link
Member

blyxyas commented Nov 11, 2023

? It didn't push?

@blyxyas
Copy link
Member

blyxyas commented Nov 11, 2023

@bors r+

@bors
Copy link
Collaborator

bors commented Nov 11, 2023

💡 This pull request was already approved, no need to approve it again.

@bors
Copy link
Collaborator

bors commented Nov 11, 2023

📌 Commit 4dead77 has been approved by blyxyas

It is now in the queue for this repository.

@bors
Copy link
Collaborator

bors commented Nov 11, 2023

⌛ Testing commit 4dead77 with merge 8ee9a9c...

@bors
Copy link
Collaborator

bors commented Nov 11, 2023

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: blyxyas
Pushing 8ee9a9c to master...

@bors bors merged commit 8ee9a9c into rust-lang:master Nov 11, 2023
5 checks passed
@matthri matthri deleted the unnecessary-fallible-conversions-ext-notes branch November 22, 2023 22:16
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.

unnecessary_fallible_conversions could show some more details
4 participants