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 lint [bool_to_int_with_if] #9086

Closed
wants to merge 37 commits into from
Closed

Add lint [bool_to_int_with_if] #9086

wants to merge 37 commits into from

Conversation

jst-r
Copy link

@jst-r jst-r commented Jul 1, 2022 •
edited
Loading

If you added a new lint, here's a checklist for things that will be
checked during review or continuous integration.

  • Followed [lint naming conventions][lint_naming]
  • Added passing UI tests (including committed .stderr file)
  • cargo test passes locally
  • Executed cargo dev update_lints
  • Added lint documentation
  • Run cargo dev fmt

fixes #8131

Please write a short comment explaining your change (or "none" for internal only changes)

changelog: added lint [`bool_to_int_with_if`]

@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 @dswij (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 Jul 1, 2022
@jst-r jst-r changed the title Add lint bool_to_int_with_if Add lint [bool_to_int_with_if] Jul 1, 2022
xFrednet
xFrednet reviewed Jul 1, 2022
View reviewed changes
tests/ui/author/struct.rs Outdated Show resolved Hide resolved
@xFrednet
Copy link
Member

xFrednet commented Jul 1, 2022

This looks like a fun one, and I've already had a discussion in the issue. I'm claiming the review, I hope that's okay.

r? @xFrednet

I'll do a full review over the weekend.

@jst-r The commits use a different username and email than your account. This is totally fine for us, but I wanted to point that out, in case that was a mistake 🙃

@rust-highfive rust-highfive assigned xFrednet and unassigned dswij Jul 1, 2022
@xFrednet
Copy link
Member

Hey @jst-r, this is a ping from triage. Are you stuck somewhere or need more help with the added comments?

@rustbot author

@rustbot rustbot added S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties labels Jul 15, 2022
@jst-r
Copy link
Author

jst-r commented Jul 17, 2022

@xFrednet
Everything is pretty clear. Sorry for going silent. I'll finish working on your suggestions this week. If I get stuck on anything I'll let you know.

Hey @jst-r, this is a ping from triage. Are you stuck somewhere or need more help with the added comments?

@xFrednet
Copy link
Member

@rustbot ready

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties and removed S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) labels Jul 27, 2022
xFrednet
xFrednet approved these changes Aug 4, 2022
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.

The implementation looks good to me! I have some small formatting nits which should be simple to fix. Then the implementation is ready 🎉

Before we can merge it, there are two more things. Rust and Clippy use a No merge commit policy. This basically means that then only merge commits should be added for PRs. Conflicts have to be resolved with rebases. See resolve the merge conflicts. And it would be good if you could squash most commits, so we only have some few meaningful ones left.

To avoid dropping the merge commits first etc. you can squash all commits, including the merge commits, into one. Then you don't have to deal with them separately. You're welcome to reach out, if you need any help with that :)

clippy_lints/src/bool_to_int_with_if.rs Outdated Show resolved Hide resolved
clippy_lints/src/bool_to_int_with_if.rs Show resolved Hide resolved
clippy_lints/src/bool_to_int_with_if.rs Outdated Show resolved Hide resolved
clippy_lints/src/bool_to_int_with_if.rs Outdated Show resolved Hide resolved
clippy_lints/src/bool_to_int_with_if.rs Outdated Show resolved Hide resolved
clippy_lints/src/bool_to_int_with_if.rs Show resolved Hide resolved
@xFrednet
Copy link
Member

xFrednet commented Aug 4, 2022

@rustbot author

@rustbot rustbot added S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties labels Aug 4, 2022
@xFrednet
Copy link
Member

xFrednet commented Aug 13, 2022 •
edited
Loading

Hey @jst-r, the last version looks good to me. Now all that needs to be done is a rebase and squashing some commits as well as removing the merge commits. I would suggest squashing everything into one, large commit, as that would also remove the merge commits. Let me know if you need any help with that 🙃.

@xFrednet
Copy link
Member

xFrednet commented Sep 1, 2022

Hey @jst-r, I would love to get this merged, since the lint is a nice addition to Clippy. Since you haven't replied, would it be alright with you, if I squash the commits and merge it? 🙃

@jst-r
Copy link
Author

jst-r commented Sep 1, 2022

Hi, it would be great if you could merge it. Sorry for dissappearing again, deadlines tied to back to school got the best of me.

@xFrednet xFrednet mentioned this pull request Sep 1, 2022
Merged
bors added a commit that referenced this pull request Sep 1, 2022
@bors
Auto merge of #9412 - xFrednet:jst-r-bool-to-int-lint, r=xFrednet
334be18 
New lint `bool_to_int_with_if`

This is a rebased version of #9086 I could sadly not push directly push to the PR branch as it's protected.

The lint implementation comes from `@jst-r.` Thank you for the work you put into this :)

---

Closes: #8131
Closes: #9086

changelog: Add lint [`bool_to_int_with_if`]

r? `@ghost`
@xFrednet
Copy link
Member

xFrednet commented Sep 1, 2022

Alright, I'm closing this in favor of #9412. Sadly I couldn't push to this branch since your master is protected. I hope that everything works out and you get credit for the commit. The rebase actually took some time, but the test are all passing now.

Hi, it would be great if you could merge it. Sorry for dissappearing again, deadlines tied to back to school got the best of me.

No worries, that also happens to me. I hope that you still enjoyed working on Clippy. Thank you for implementing this lint :)

@xFrednet xFrednet closed this Sep 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@xFrednet xFrednet xFrednet approved these changes

Assignees

@xFrednet xFrednet

Labels
S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status)
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Suggest casting bool to int instead of using if statement.
6 participants
@jst-r @rust-highfive @xFrednet @bors @dswij @rustbot