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 new lint seek_to_start_instead_of_rewind #9667

Merged
merged 3 commits into from
Oct 25, 2022
Merged

add new lint seek_to_start_instead_of_rewind #9667

merged 3 commits into from
Oct 25, 2022

Conversation

dblnz
Copy link
Contributor

@dblnz dblnz commented Oct 17, 2022 •
edited by giraffate
Loading

changelog: seek_to_start_instead_of_rewind: new lint to suggest using rewind instead of seek to start

Resolve #8600

@rust-highfive
Copy link

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

Please see the contribution instructions for more information.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Oct 17, 2022
giraffate
giraffate reviewed Oct 20, 2022
View reviewed changes
Copy link
Contributor

@giraffate giraffate left a comment

Choose a reason for hiding this comment

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

Sorry, I forgot msrv in the first review. The rewind was implemented at 1.55.0, so this need to implement msrv: https://github.com/rust-lang/rust-clippy/blob/master/book/src/development/adding_lints.md#specifying-the-lints-minimum-supported-rust-version-msrv

The PR below would be helpful to implement msvr: #8692

@koka831 koka831 mentioned this pull request Oct 20, 2022
Merged
@bors
Copy link
Collaborator

bors commented Oct 23, 2022

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

@bors
Copy link
Collaborator

bors commented Oct 23, 2022

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

@giraffate
Copy link
Contributor

I'll merge this once the conflicts are resolved.

@dblnz
Add new lint rewind_instead_of_seek_to_start
8d6ce31 
Signed-off-by: Doru-Florin Blanzeanu <blanzeanu.doru@protonmail.com>
@dblnz
Add msrv check for rewind_instead_of_seek_to_start lint
b48a466 
Signed-off-by: Doru-Florin Blanzeanu <blanzeanu.doru@protonmail.com>
@Alexendoo
Copy link
Member

Alexendoo commented Oct 24, 2022 •
edited
Loading

Jumping in late here but I think the lint name should be manual_rewind or similar, it's currently reversed as it's saying what to do rather than what the issue is

Also sorry for causing you two conflicts in 24 hours 😄

@dblnz
Copy link
Contributor Author

dblnz commented Oct 24, 2022

It depends on how you look at it, but now that you pointed it out, it looks odd.
manual_rewind does not seem clearer to me as allow/deny manual_rewind seems to be doing something else. What do you say if I reverse it to seek_to_start_instead_of_rewind ? It is a bit long, but to me is clearer.

No worries about the conflicts 😄

@Alexendoo
Copy link
Member

Yeah that's fine by me

@dblnz
Change lint name to seek_to_start_instead_of_rewind
b9b9d6a 
- This name makes more sense and highlights the issue

Signed-off-by: Doru-Florin Blanzeanu <blanzeanu.doru@protonmail.com>
@giraffate giraffate changed the title add new lint rewind_instead_of_seek_to_start add new lint seek_to_start_instead_of_rewind Oct 25, 2022
@giraffate
Copy link
Contributor

@Alexendoo Thanks for your review!

@giraffate
Copy link
Contributor

@bors r+

Thanks!

@bors
Copy link
Collaborator

bors commented Oct 25, 2022

📌 Commit b9b9d6a has been approved by giraffate

It is now in the queue for this repository.

@bors
Copy link
Collaborator

bors commented Oct 25, 2022

⌛ Testing commit b9b9d6a with merge 039af9c...

@bors
Copy link
Collaborator

bors commented Oct 25, 2022

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: giraffate
Pushing 039af9c to master...

@bors bors merged commit 039af9c into rust-lang:master Oct 25, 2022
@bors bors mentioned this pull request Oct 25, 2022
Closed
bors added a commit that referenced this pull request Oct 26, 2022
@bors
Auto merge of #9681 - koka831:feat/add-seek-from-current-lint, r=gira…
7182a6b 
…ffate

feat: add new lint `seek_from_current`

changelog: `seek_from_current`: new lint to suggest using `stream_position` instead of seek from current position with `SeekFrom::Current(0)`

addresses #7886.

This PR is related to #9667, so I will update `methods/mod.rs` if it get conflicted.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@giraffate giraffate giraffate approved these changes

Assignees

@giraffate giraffate

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.

Prefer Seek#rewind() over Seek#seek(SeekFrom::Start(0))
5 participants
@dblnz @rust-highfive @bors @giraffate @Alexendoo