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 lint almost_complete_digit_range #10043

Merged
merged 1 commit into from Dec 9, 2022
Merged

Add lint almost_complete_digit_range #10043

merged 1 commit into from Dec 9, 2022

Conversation

phtown
Copy link
Contributor

@phtown phtown commented Dec 6, 2022

changelog: [almost_complete_digit_range]: Add digit analog to almost_complete_letter_range

I have added a lint that will detect '0'..'9' and suggest converting it to '0'..='9', the same way almost_complete_letter_range does for ascii letters. I tied into the implementation of AlmostCompleteLetterRange in order to do that, but I didn't change its interface.
This is my first contribution to Clippy, so please let me know if there's anything I should do differently. I'll be happy to incorporate any suggestions you have.
Thanks!

@rustbot
Copy link
Collaborator

rustbot commented Dec 6, 2022

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

Please see the contribution instructions for more information.

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Dec 6, 2022
@Jarcho
Copy link
Contributor

Jarcho commented Dec 8, 2022

Just a comment on naming. It would be better to rename almost_complete_letter_range to almost_complete_range, and then use the same lint to implement both. Changes look good otherwise.

Might be easier to start from master, use cargo dev rename_lint almost_complete_letter_range almost_complete_range and then add your changes onto that.

@phtown
Copy link
Contributor Author

phtown commented Dec 8, 2022

While making the change you suggested I ran into an issue I hope you can shed some light on. If you run the test tests/ui/almost_complete_range and look at the macro b, you'll see the lint only triggers on the first instance it should. Is that expected behavior for macro_rules macros? I can't see anything in the lint definition that would make it do that.

@Jarcho
Copy link
Contributor

Jarcho commented Dec 8, 2022

I'm surprised the compiler isn't complaining about that. Only the first arm of that macro will ever be expanded.

You probably meant to do something like

macro_rules! b {
    () => {{
        let _ = 'a'..'z';
        let _ = 'A'..'Z';
        let _ = '0'..'9';
    }};
}

@phtown
Copy link
Contributor Author

phtown commented Dec 8, 2022

Oh, woops. Looks like rustc and I are equally good at noticing when a macro definition is wrong.

@Jarcho
Copy link
Contributor

Jarcho commented Dec 9, 2022

Looks good to go. You can squash the changes. Probably best to have the rename as a separate commit, but not necessary if that's too much effort.

This replaces and expands `almost_complete_letter_range`.
@phtown phtown reopened this Dec 9, 2022
@Jarcho
Copy link
Contributor

Jarcho commented Dec 9, 2022

Weird. I wonder what you did.

Thank you. @bors r+

@bors
Copy link
Collaborator

bors commented Dec 9, 2022

📌 Commit 4c80f21 has been approved by Jarcho

It is now in the queue for this repository.

@bors
Copy link
Collaborator

bors commented Dec 9, 2022

⌛ Testing commit 4c80f21 with merge ef2018c...

@bors
Copy link
Collaborator

bors commented Dec 9, 2022

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

@bors bors merged commit ef2018c into rust-lang:master Dec 9, 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

4 participants