Skip to content

Modify str_to_string to be machine-applicable #12871

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

Merged
merged 1 commit into from
Jun 2, 2024

Conversation

Jacherr
Copy link
Contributor

@Jacherr Jacherr commented May 30, 2024

Fixes #12768

I'm not sure if there is any potential for edge cases with this - since it only ever acts on &str types I can't think of any, and especially since the methods do the same thing anyway.

changelog: allow str_to_string lint to be automatically applied

@rustbot
Copy link
Collaborator

rustbot commented May 30, 2024

r? @blyxyas

rustbot has assigned @blyxyas.
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 May 30, 2024
Comment on lines 410 to 411
"consider using `.to_owned()`",
format!("{snippet}.to_owned()"),
Copy link
Member

@J-ZhengLi J-ZhengLi May 31, 2024

Choose a reason for hiding this comment

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

I think having two .to_owned() here seems a bit redundant? How about just using try as the help msg instead?

"try",
format!("{snippet}.to_owned()"),

Copy link
Member

Choose a reason for hiding this comment

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

(drive-by comment)

It's worth noting that the rustc-dev-guide explicitly state that the suggestion message should "be upfront about what the suggestion is" and explicitly say to avoid "did you mean"/"the following" ...

I would also note that in IDEs, users may not have access to the terminal output of the compiler and may only have access to the help message to decide if they should apply the suggestion and "try" is less than helpful is those cases.

Copy link
Member

@J-ZhengLi J-ZhengLi Jun 3, 2024

Choose a reason for hiding this comment

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

...the suggestion message should "be upfront about what the suggestion is"...

Well, my bad, I didn't know that 👀.

But in my defense, there are already fair amount of suggestions with "try" existing in clippy. like nonminimal_bool, manual_swap, etc. (there are 170+ of them I think), so I though it would make sense to use it.

...in IDEs, users may not have access to the terminal output of the compiler and may only have access to the help message to decide if they should apply the suggestion and "try" is less than helpful is those cases.

In that case, maybe clippy could use a "clean up" to replace those existing "try"s (including this one) with more helpful messages? But I don't know if that worth the effort.

Thanks for the heads up~

Copy link
Member

Choose a reason for hiding this comment

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

No worries, we also have in rustc messages that don't follow our own guidelines, so I wouldn't expect Clippy or other tools to follow it to the letter either.

In that case, maybe clippy could use a "clean up" to replace those existing "try"s (including this one) with more helpful messages? But I don't know if that worth the effort.

I'm not on the Clippy team so you would have to ask them out that.
Personally I think that could be long term goal.

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 Jun 2, 2024

@bors r+

@bors
Copy link
Contributor

bors commented Jun 2, 2024

📌 Commit 5d0fcfb has been approved by blyxyas

It is now in the queue for this repository.

@bors
Copy link
Contributor

bors commented Jun 2, 2024

⌛ Testing commit 5d0fcfb with merge 568f4fc...

@bors
Copy link
Contributor

bors commented Jun 2, 2024

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

@bors bors merged commit 568f4fc into rust-lang:master Jun 2, 2024
5 checks passed
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.

Add fix for str_to_string
6 participants