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

Rename collapsable_if fix suggestion to "collapse nested if block" #5732

Merged
merged 1 commit into from
Jul 14, 2020
Merged

Rename collapsable_if fix suggestion to "collapse nested if block" #5732

merged 1 commit into from
Jul 14, 2020

Conversation

bjorn3
Copy link
Member

@bjorn3 bjorn3 commented Jun 19, 2020

The name "try" is confusing when shown as quick fix by rust-analyzer

changelog: Rename collapsable_if fix suggestion to "collapse nested if block"

@rust-highfive
Copy link

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @yaahc (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Jun 19, 2020
@mati865
Copy link
Contributor

mati865 commented Jun 19, 2020

I think this is how most of Clippy lints look like.
Even doc span_lint_and_sugg does that:

/// In the example below, `help` is `"try"` and `sugg` is the suggested replacement `".any(|x| x >

@bjorn3
Copy link
Member Author

bjorn3 commented Jun 19, 2020

Having a generic name like try is very confusing in ide cases. And if there are multiple lints that trigger on the same piece of code, there is literally no way to know which try applies which suggestion.

@mati865
Copy link
Contributor

mati865 commented Jun 19, 2020

I'm not opposed. Fixing all the lints will require a lot of work though.

@flip1995
Copy link
Member

You will need to update the stderr reference files. You can do this by running cargo uitest followed by sh tests/ui/update-all-references.sh. Everything else LGTM.

The name "try" is confusing when shown as quick fix by rust-analyzer

I will take this in consideration when reviewing future lints, thanks for the feedback!

r? @flip1995

@rust-highfive rust-highfive assigned flip1995 and unassigned yaahc Jun 22, 2020
@flip1995 flip1995 added S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties labels Jun 22, 2020
@flip1995
Copy link
Member

Friendly ping @bjorn3 . Can you update the stderr files, so we can merge this?

The name "try" is confusing when shown as quick fix by rust-analyzer
@bjorn3
Copy link
Member Author

bjorn3 commented Jul 13, 2020

Thanks for the ping. Updated the stderr files, rebased and squashed.

@flip1995
Copy link
Member

Thanks!

@bors r+

@bors
Copy link
Contributor

bors commented Jul 14, 2020

📌 Commit ff796b6 has been approved by flip1995

@bors
Copy link
Contributor

bors commented Jul 14, 2020

⌛ Testing commit ff796b6 with merge 2ca58e7...

@bors
Copy link
Contributor

bors commented Jul 14, 2020

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: flip1995
Pushing 2ca58e7 to master...

@bors bors merged commit 2ca58e7 into rust-lang:master Jul 14, 2020
@bjorn3 bjorn3 deleted the patch-1 branch July 14, 2020 10:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants