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.

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

New lint [min_ident_chars] #10916

Merged
merged 8 commits into from
Jun 12, 2023
Merged

New lint [min_ident_chars] #10916

merged 8 commits into from
Jun 12, 2023

Conversation

Centri3
Copy link
Member

@Centri3 Centri3 commented Jun 9, 2023 •
edited by xFrednet
Loading

Closes #10915

This also implements WithSearchPat for Ident, as I was going to rewrite this as a late lint to optionally ignore fields and/or generics but this was more complex than anticipated

changelog: New lint [min_ident_chars]
#10916

@rustbot
Copy link
Collaborator

rustbot commented Jun 9, 2023

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 Jun 9, 2023
@xFrednet
Copy link
Member

xFrednet commented Jun 9, 2023

Hey @blyxyas do you want to review this PR? It sounds like a good and simple lint :)

Assuming it's fine for @Jarcho if we steal the review.

r? @xFrednet

@rustbot rustbot assigned xFrednet and unassigned Jarcho Jun 9, 2023
@blyxyas
Copy link
Member

blyxyas commented Jun 9, 2023

Yes, will take it :)

@Centri3 Centri3 force-pushed the single_letter_idents branch from 1a6e9f5 to e297fbe Compare June 9, 2023 18:53
@Centri3 Centri3 changed the title New lint [single_letter_idents] New lint [single_char_idents] Jun 9, 2023
blyxyas
blyxyas reviewed Jun 9, 2023
View reviewed changes
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.

Some really really nitpicks. Everything else looks fine, Thanks ❤️

clippy_lints/src/single_letter_idents.rs Outdated Show resolved Hide resolved
clippy_lints/src/single_letter_idents.rs Outdated Show resolved Hide resolved
clippy_lints/src/utils/conf.rs Outdated Show resolved Hide resolved
@Centri3 Centri3 force-pushed the single_letter_idents branch 3 times, most recently from a2c037f to 3defd38 Compare June 9, 2023 23:03
blyxyas
blyxyas approved these changes Jun 10, 2023
View reviewed changes
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.

Everything seems pretty fine, thanks! Seems like a very useful lint ❤️

cc @xFrednet

@Centri3 Centri3 force-pushed the single_letter_idents branch from b6cc2fb to 3d0adf9 Compare June 10, 2023 13:12
@xFrednet
Copy link
Member

xFrednet commented Jun 10, 2023 •
edited
Loading

You just pushed, while I was doing a review xD

It looks like that deleted a comment, let's see if I can find that one again

@Centri3
Copy link
Member Author

Centri3 commented Jun 10, 2023

Sorry, had to make sure I ran collect-metadata on this one (spoiler alert: I did not.)

xFrednet
xFrednet reviewed Jun 10, 2023
View reviewed changes
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.

Hey, so far, I've only looked at the tests and config implementation. It looks like you're also still working on the branch. These are some nits I found rn. Let me know when you're ready for the next review :)

clippy_lints/src/utils/conf.rs Show resolved Hide resolved
tests/ui-toml/min_ident_chars/clippy.toml Show resolved Hide resolved
tests/ui/min_ident_chars.rs Show resolved Hide resolved
tests/ui/min_ident_chars.rs Show resolved Hide resolved
@xFrednet
Copy link
Member

Sorry, had to make sure I ran collect-metadata on this one (spoiler alert: I did not.)

No problem! It was not the first time this happened, just the first time that GH deleted a comment. It was a simple one to reconstruct though :D

@Centri3
Copy link
Member Author

Centri3 commented Jun 10, 2023

Let me know when you're ready for the next review :)

Feel free to take a look whenever, the implementation is already pretty much done.

@xFrednet
Copy link
Member

I looks like the PR has gotten some conflict, without bors adding a comment. Could you resolve them?

@Centri3 Centri3 force-pushed the single_letter_idents branch from c98e566 to 76996cb Compare June 10, 2023 14:35
xFrednet
xFrednet reviewed Jun 11, 2023
View reviewed changes
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 version looks quite good to me. I have three tiny nits, and then it should be ready :)

clippy_lints/src/min_ident_chars.rs Outdated Show resolved Hide resolved
clippy_lints/src/min_ident_chars.rs Outdated Show resolved Hide resolved
clippy_lints/src/min_ident_chars.rs Outdated Show resolved Hide resolved
@bors
Copy link
Contributor

bors commented Jun 12, 2023

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

@Centri3 Centri3 changed the title New lint [single_char_idents] New lint [min_ident_chars] Jun 12, 2023
@Centri3 Centri3 changed the title New lint [min_ident_chars] New lint [min_ident_chars] Jun 12, 2023
@Centri3 Centri3 force-pushed the single_letter_idents branch from 99f9c12 to 29c1c6e Compare June 12, 2023 08:22
@xFrednet
Copy link
Member

Looks good to me, thank you!

@bors r=blyxyas,xFrednet

@bors
Copy link
Contributor

bors commented Jun 12, 2023

📌 Commit 29c1c6e has been approved by blyxyas,xFrednet

It is now in the queue for this repository.

@bors
Copy link
Contributor

bors commented Jun 12, 2023

⌛ Testing commit 29c1c6e with merge a3b185b...

@bors
Copy link
Contributor

bors commented Jun 12, 2023

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

@bors bors merged commit a3b185b into rust-lang:master Jun 12, 2023
@Centri3 Centri3 deleted the single_letter_idents branch June 12, 2023 09:06
This was referenced Jun 12, 2023
Merged
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@blyxyas blyxyas blyxyas approved these changes

@xFrednet xFrednet xFrednet approved these changes

Assignees

@xFrednet xFrednet

@blyxyas blyxyas

Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

single_letter_ident
6 participants
@Centri3 @rustbot @xFrednet @blyxyas @bors @Jarcho