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 unwrap_or_else_default lint #7516

Merged
merged 4 commits into from
Aug 12, 2021
Merged

Conversation

lf-
Copy link
Contributor

@lf- lf- commented Jul 30, 2021


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

changelog: Add a new [unwrap_or_else_default] style lint. This will catch unwrap_or_else(Default::default) on Result and Option and suggest unwrap_or_default() instead.

@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 @llogiq (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 30, 2021
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, thank you very much for your work! These are some suggestions of further improvements. The rest of the implementation looks good IMO, and you also already added the lint in the right place 👍

clippy_lints/src/methods/mod.rs Outdated Show resolved Hide resolved
clippy_lints/src/methods/mod.rs Outdated Show resolved Hide resolved
clippy_lints/src/methods/unwrap_or_else_default.rs Outdated Show resolved Hide resolved
clippy_lints/src/methods/unwrap_or_else_default.rs Outdated Show resolved Hide resolved
tests/ui/unwrap_or_else_default.rs Show resolved Hide resolved
tests/ui/unwrap_or_else_default.rs Outdated Show resolved Hide resolved
clippy_lints/src/methods/unwrap_or_else_default.rs Outdated Show resolved Hide resolved
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.

Thank you for the PR update. I've marked a few things which could be improved, the rest of the code already looks good IMO. 🙃

clippy_lints/src/methods/mod.rs Outdated Show resolved Hide resolved
clippy_lints/src/methods/mod.rs Outdated Show resolved Hide resolved
clippy_utils/src/ty.rs Outdated Show resolved Hide resolved
clippy_utils/src/ty.rs Outdated Show resolved Hide resolved
@lf-
Copy link
Contributor Author

lf- commented Aug 6, 2021

Changes done :)

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.

Awesome, thank you! There are three more small things, and then I think it's ready to be merged.

  1. The NIT marked below
  2. Could you squash the development commits in the middle of your PR together?
  3. And also move the change log entry in the PR description on the same line as the changelog: label? (That will save work later on for the one writing it)

That should do it, IMO 🙃.

clippy_utils/src/lib.rs Outdated Show resolved Hide resolved
lf- added 3 commits August 10, 2021 14:40
This will catch `unwrap_or_else(Default::default)` on Result and Option
and suggest `unwrap_or_default()` instead.
@lf- lf- force-pushed the unwrap-or-default branch from 1afb104 to 23d398b Compare August 10, 2021 21:40
clippy_utils/src/lib.rs Outdated Show resolved Hide resolved
@xFrednet
Copy link
Member

Awesome, thank you very much for the contribution! 🙃

@bors r+

@bors
Copy link
Contributor

bors commented Aug 12, 2021

📌 Commit 295df88 has been approved by xFrednet

@bors
Copy link
Contributor

bors commented Aug 12, 2021

⌛ Testing commit 295df88 with merge e62a6ca...

@bors
Copy link
Contributor

bors commented Aug 12, 2021

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

@bors bors merged commit e62a6ca into rust-lang:master Aug 12, 2021
@flip1995
Copy link
Member

tree-wide: Fix all the rustdoc warnings

Thanks for this clean-up! Things like this should go in their own PR though. One PR should be about one thing only. Hiding such things in unrelated PRs could be confusing when you have to find out what changes were done where.

That being said: How did you find these? We may want to add checks for this to CI.

@lf-
Copy link
Contributor Author

lf- commented Aug 16, 2021

tree-wide: Fix all the rustdoc warnings

Thanks for this clean-up! Things like this should go in their own PR though. One PR should be about one thing only. Hiding such things in unrelated PRs could be confusing when you have to find out what changes were done where.

That being said: How did you find these? We may want to add checks for this to CI.

I will keep this in mind in the future. I found these partially because I like cookies and run rust nightly, and there have been some lints added to rustdoc in the current nightly rust version, and partially because I was reading the docs in question.

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.

6 participants