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

add a new lint for repeat().take() that can be replaced with repeat_n() #13858

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

add a new lint for repeat().take() that can be replaced with repeat_n() #13858

wants to merge 2 commits into from
+190 −22

Conversation

lapla-cogito
Copy link
Contributor

@lapla-cogito lapla-cogito commented Dec 20, 2024 •
edited
Loading

close #13271

changelog: add new manual_repeat_n lint

@rustbot
Copy link
Collaborator

rustbot commented Dec 20, 2024

r? @dswij

rustbot has assigned @dswij.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Dec 20, 2024
@lapla-cogito lapla-cogito force-pushed the repeatn branch 2 times, most recently from 55203fc to 33a5b21 Compare December 20, 2024 13:45
lapla-cogito
lapla-cogito commented Dec 20, 2024
View reviewed changes
clippy_utils/src/lib.rs Outdated Show resolved Hide resolved
y21
y21 reviewed Dec 20, 2024
View reviewed changes
clippy_lints/src/methods/mod.rs Outdated Show resolved Hide resolved
clippy_utils/src/lib.rs Outdated Show resolved Hide resolved
clippy_lints/src/methods/redundant_repeat_take.rs Outdated Show resolved Hide resolved
clippy_lints/src/methods/redundant_repeat_take.rs Outdated Show resolved Hide resolved
clippy_lints/src/methods/redundant_repeat_take.rs Outdated Show resolved Hide resolved
tests/ui/manual_str_repeat.rs Outdated Show resolved Hide resolved
clippy_lints/src/methods/mod.rs Outdated Show resolved Hide resolved
clippy_lints/src/methods/redundant_repeat_take.rs Outdated Show resolved Hide resolved
@lapla-cogito lapla-cogito force-pushed the repeatn branch 2 times, most recently from f3ac7b1 to 8e422ea Compare December 20, 2024 15:52
@lapla-cogito lapla-cogito changed the title add a lint for redundant repeat().take() add a new lint for repeat().take() that can be replaced with repeat_n() Dec 20, 2024
@lapla-cogito lapla-cogito force-pushed the repeatn branch 3 times, most recently from 546bbf1 to 672d7c0 Compare December 20, 2024 16:25
@lapla-cogito
Copy link
Contributor Author

@y21 Thank you for your thorough review! Looks like I was able to fix the reviewed items. Could you please take a look again?

lapla-cogito
lapla-cogito commented Dec 20, 2024
View reviewed changes
Copy link
Contributor Author

@lapla-cogito lapla-cogito left a comment •
edited
Loading

Choose a reason for hiding this comment

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

I noticed this lint should suggest to use core::iter::repeat_n in a no_std environment. Therefore, I'll make a change.

@lapla-cogito lapla-cogito requested a review from y21 December 20, 2024 17:10
@lapla-cogito
Copy link
Contributor Author

r? @y21
This is not meant to prompt a review, but simply to switch the assignee.

@rustbot rustbot assigned y21 and unassigned dswij Dec 26, 2024
@rustbot rustbot added has-merge-commits PR has merge commits, merge with caution. S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) labels Jan 2, 2025
@rustbot

This comment has been minimized.

Sign in to view
@rustbot rustbot removed has-merge-commits PR has merge commits, merge with caution. S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) labels Jan 2, 2025
@lapla-cogito
Copy link
Contributor Author

Conflicts now resolved.

y21
y21 approved these changes Jan 3, 2025
View reviewed changes
Copy link
Member

@y21 y21 left a comment

Choose a reason for hiding this comment

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

Looks basically good to me, just a few small suggestions then it's ready for the FCP I would say

clippy_lints/src/methods/manual_repeat_n.rs Outdated Show resolved Hide resolved
clippy_lints/src/methods/manual_repeat_n.rs Outdated Show resolved Hide resolved
clippy_lints/src/methods/manual_repeat_n.rs Outdated Show resolved Hide resolved
clippy_lints/src/methods/manual_repeat_n.rs Outdated Show resolved Hide resolved
clippy_lints/src/methods/mod.rs Outdated Show resolved Hide resolved
@lapla-cogito
Copy link
Contributor Author

@y21 Thank you for your review! All of your suggestions seem to have been resolved now.

@lapla-cogito lapla-cogito requested a review from y21 January 4, 2025 11:25
@lapla-cogito
Copy link
Contributor Author

Any chance of merging this?

@lapla-cogito
Copy link
Contributor Author

@y21 Updated the changelog line to deal with the updated CI that checks the contents of it.

@y21
Copy link
Member

y21 commented Jan 12, 2025

Hey, sorry for the long wait here. This all looks good to me, thank you! I've created a thread on Zulip for the usual FCP process: https://rust-lang.zulipchat.com/#narrow/channel/257328-clippy/topic/FCP.3A.20manual_repeat_n

If nothing comes up, we can merge it

@y21 y21 added S-final-comment-period Status: final comment period it will be merged unless new objections are raised (~1 week) and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties labels Jan 12, 2025
y21
y21 reviewed Jan 12, 2025
View reviewed changes
Copy link
Member

@y21 y21 left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Actually, one very small thing that I forgot to comment on. Also would be nice if you could squash your commits a bit :)

clippy_lints/src/methods/mod.rs Outdated Show resolved Hide resolved
@lapla-cogito lapla-cogito force-pushed the repeatn branch 2 times, most recently from 50635d6 to 25f75ff Compare January 15, 2025 04:13
@lapla-cogito
Copy link
Contributor Author

Conflicts resolved.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@y21 y21 y21 approved these changes

Assignees

@y21 y21

Labels
S-final-comment-period Status: final comment period it will be merged unless new objections are raised (~1 week)
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

iter::repeat(copy_value).take(n) => repeat_n(copy_value, n)
4 participants
@lapla-cogito @rustbot @y21 @dswij