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

improve manual_is_ascii_check check #10053

Merged
merged 5 commits into from
Dec 14, 2022
Merged

improve manual_is_ascii_check check #10053

merged 5 commits into from
Dec 14, 2022

Conversation

naosense
Copy link
Contributor

@naosense naosense commented Dec 9, 2022

Sorry, not familiar the api, i can only check the method name of expression <expr-1>.contains(<expr-2>) after read clippy book and hints from #9933 . i dont know how to check

  1. if is a specific range
  2. is a character

r? @xFrednet could you please provide some more hints? 😝️


changelog: Enhancement: [manual_is_ascii_check]: Now detects ranges with .contains() calls
#10053

@rustbot
Copy link
Collaborator

rustbot commented Dec 9, 2022

r? @Jarcho

(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 Dec 9, 2022
@rustbot rustbot assigned xFrednet and unassigned Jarcho Dec 9, 2022
@llogiq
Copy link
Contributor

llogiq commented Dec 9, 2022

  1. We have a clippy_utils::higher::Range::hir(&Expr) call that will give you a higher::Range { start, end, limits } you can use to match both exclusive and inclusive ranges.
  2. you'll be looking at a ExprKind::Lit with a node of LitKind::Char(c) containing the character.

@naosense
Copy link
Contributor Author

@llogiq Can you take a look at whether I'm in the right direction?

Copy link
Member

@xFrednet xFrednet left a comment

Choose a reason for hiding this comment

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

This looks like a good start. The main thing missing are tests. I've added comments at the related locations :)

clippy_lints/src/manual_is_ascii_check.rs Outdated Show resolved Hide resolved
tests/ui/manual_is_ascii_check.rs Outdated Show resolved Hide resolved
@naosense naosense marked this pull request as ready for review December 13, 2022 03:00
@naosense naosense changed the title add contains check improve manual_is_ascii_check check Dec 13, 2022
Copy link
Member

@xFrednet xFrednet left a comment

Choose a reason for hiding this comment

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

The implementation looks good to me, three more nits and then it's ready for merge. Would you mind squashing your commits a bit? Like the two ones adding tests and the two first ones with the same message. :)

clippy_lints/src/manual_is_ascii_check.rs Outdated Show resolved Hide resolved
clippy_lints/src/manual_is_ascii_check.rs Outdated Show resolved Hide resolved
tests/ui/manual_is_ascii_check.rs Outdated Show resolved Hide resolved
@xFrednet
Copy link
Member

Looks good to me, thank you for the addition. I hope you had fun :)

@bors r+

@bors
Copy link
Collaborator

bors commented Dec 14, 2022

📌 Commit 1f862c2 has been approved by xFrednet

It is now in the queue for this repository.

@bors
Copy link
Collaborator

bors commented Dec 14, 2022

⌛ Testing commit 1f862c2 with merge be15e60...

@bors
Copy link
Collaborator

bors commented Dec 14, 2022

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

@bors bors merged commit be15e60 into rust-lang:master Dec 14, 2022
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.

None yet

6 participants