Skip to content

Lint collapsible_str_replace #9269

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 7 commits into from
Aug 20, 2022
Merged

Lint collapsible_str_replace #9269

merged 7 commits into from
Aug 20, 2022

Conversation

nahuakang
Copy link
Contributor

@nahuakang nahuakang commented Jul 31, 2022 •
edited
Loading

fixes #6651

changelog: [`collapsible_str_replace`]: create new lint `collapsible_str_replace`

If you added a new lint, here's a checklist for things that will be
checked during review or continuous integration.

  • Followed [lint naming conventions][lint_naming]
  • Added passing UI tests (including committed .stderr file)
  • cargo test passes locally
  • Executed cargo dev update_lints
  • Added lint documentation
  • Run cargo dev fmt

@rust-highfive
Copy link

r? @flip1995

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Jul 31, 2022
flip1995
flip1995 reviewed Aug 2, 2022
View reviewed changes
Copy link
Member

@flip1995 flip1995 left a comment

Choose a reason for hiding this comment

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

Only one comment about the test, otherwise the tests LGTM.

This could be included in the methods module. Then you already get the method matching for free. You'll also have to check the type of the receiver and check if it is a string-ish (&str, String, ...) type. I think we have a helper for this(?).

@bors
Copy link
Contributor

bors commented Aug 2, 2022

☔ The latest upstream changes (presumably #9264) made this pull request unmergeable. Please resolve the merge conflicts.

@nahuakang nahuakang marked this pull request as ready for review August 8, 2022 20:32
@nahuakang nahuakang requested a review from flip1995 August 9, 2022 06:55
flip1995
flip1995 requested changes Aug 14, 2022
View reviewed changes
Copy link
Member

@flip1995 flip1995 left a comment

Choose a reason for hiding this comment

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

This implementation is way too complicated. You walk the method chain way too often with for_each_expr. You should walk it once and then return a vector of expressions to all the method calls, that you then reuse for all other checks you want to do.

I left multiple suggestions on how to simplify the code. Just comment if the suggestions are unclear or you have questions about them.

@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 Aug 14, 2022
@nahuakang nahuakang requested a review from flip1995 August 14, 2022 22:31
flip1995
flip1995 requested changes Aug 15, 2022
View reviewed changes
Copy link
Member

@flip1995 flip1995 left a comment

Choose a reason for hiding this comment

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

Some more suggestiong on how to simplify the code. Thanks for addressing this so quickly!

@nahuakang nahuakang requested a review from flip1995 August 15, 2022 14:16
flip1995
flip1995 reviewed Aug 17, 2022
View reviewed changes
Copy link
Member

@flip1995 flip1995 left a comment

Choose a reason for hiding this comment

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

This review round, that comments are more about the tests. The code looks way cleaner and easier to understand now!

@nahuakang nahuakang requested a review from flip1995 August 17, 2022 19:25
flip1995
flip1995 reviewed Aug 19, 2022
View reviewed changes
Copy link
Member

@flip1995 flip1995 left a comment

Choose a reason for hiding this comment

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

Linting logic LGTM now. If the 2 test suggestions pass, this is ready to get merged.

@bors
Copy link
Contributor

bors commented Aug 19, 2022

☔ The latest upstream changes (presumably #8957) made this pull request unmergeable. Please resolve the merge conflicts.

@nahuakang nahuakang requested a review from flip1995 August 19, 2022 18:26
@flip1995
Copy link
Member

@bors r+

Thanks! That implementation turned out nicely!

@bors
Copy link
Contributor

bors commented Aug 20, 2022

📌 Commit b070b40 has been approved by flip1995

It is now in the queue for this repository.

@bors
Copy link
Contributor

bors commented Aug 20, 2022

⌛ Testing commit b070b40 with merge 5820add...

@nahuakang
Copy link
Contributor Author

@flip1995 @xFrednet Thank you both for your patience and help 🙏
It's absolutely just wonderful to come back to the project and receive so much support and help ❤️

@bors
Copy link
Contributor

bors commented Aug 20, 2022

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

@bors bors merged commit 5820add into rust-lang:master Aug 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@flip1995 flip1995 Awaiting requested review from flip1995

Assignees

@flip1995 flip1995

Labels
S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status)
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

New lint: collapsible_str_replace
4 participants
@nahuakang @rust-highfive @bors @flip1995