Skip to content

Add [err_expect] lint - #8606

Merged
bors merged 2 commits into
masterfrom
unknown repository
Apr 6, 2022
Merged

Add [err_expect] lint#8606
bors merged 2 commits into
masterfrom
unknown repository

Conversation

@ghost

@ghost ghost commented Mar 29, 2022 •
edited by ghost
Loading

Copy link
Copy Markdown

[expect_err] lint

  • 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 #1435

changelog: Added a lint to detect usage of .err().expect()

@rust-highfive

rust-highfive commented Mar 29, 2022

Copy link
Copy Markdown

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @camsteffen (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 Mar 29, 2022
@xFrednet

xFrednet commented Mar 29, 2022

Copy link
Copy Markdown
Contributor

Very awesome, I'll assign this to myself. Right now I'm a bit tired, but I'll try to review it later today or tomorrow 🙃

r? @xFrednet

@rust-highfive rust-highfive assigned xFrednet and unassigned camsteffen Mar 29, 2022
xFrednet
xFrednet reviewed Mar 30, 2022
View reviewed changes

@xFrednet xFrednet left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This is already a great start 👍. I've added some notes for improvements and checks that need to be added. Thank you for the work, you already put into this. You're welcome to reach out if something is unclear 🙃

Comment thread clippy_lints/src/methods/mod.rs Resolved
Comment thread clippy_lints/src/methods/mod.rs Resolved
Comment thread clippy_lints/src/methods/mod.rs Resolved
Comment thread tests/ui/expect_err.rs Resolved
Comment thread clippy_lints/src/methods/expect_err.rs Resolved
Comment thread clippy_lints/src/methods/expect_err.rs Resolved
@xFrednet xFrednet 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 Mar 30, 2022
@xFrednet

xFrednet commented Mar 30, 2022

Copy link
Copy Markdown
Contributor

I put some more thought into the name. I think the current one expect_err doesn't work with lint attributes. #[deny(clippy::expect_err)] sounds like it denies the usage of expect_err instead of the other way around. Maybe err_then_expect, err_expect or manual_expect_err would be better names for the lint. 🤔 err_expect might be the best name as it is similar to ok_expect 🙃

@ghost ghost changed the title Add [expect_err] lint Add [err_expect] lint Mar 31, 2022
xFrednet
xFrednet reviewed Mar 31, 2022
View reviewed changes

@xFrednet xFrednet left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Nice improvements 👍 I've attempted to answer all your questions, if I missed something, please say so :)

Comment thread clippy_lints/src/methods/expect_err.rs Resolved
Comment thread clippy_lints/src/methods/mod.rs Resolved
Comment thread clippy_lints/src/methods/mod.rs Resolved
Comment thread tests/ui/expect_err.rs Resolved
Comment thread clippy_lints/src/methods/expect_err.rs Resolved
@ghost

This comment was marked as resolved.

Sign in to view
xFrednet
xFrednet reviewed Mar 31, 2022
View reviewed changes
Comment thread tests/ui/err_expect.rs Resolved
xFrednet
xFrednet reviewed Mar 31, 2022
View reviewed changes
Comment thread clippy_lints/src/methods/mod.rs Resolved
@ghost

This comment was marked as resolved.

Sign in to view
@xFrednet

xFrednet commented Apr 1, 2022

Copy link
Copy Markdown
Contributor

I'm not sure why this issue occurs https://github.com/rust-lang/rust-clippy/runs/5779347126?check_suite_focus=true

You've added something to tests/ui/min_rust_version_attr.rs which changes the line numbers for lint reports inside the related .stderr file. A simple cargo test and then cargo dev bless should update the .stderr file and fix the CI 🙃

@ghost

ghost commented Apr 1, 2022 •
edited by ghost
Loading

Copy link
Copy Markdown
Author

I think I went through all of your reviews, tests are passing, is this ready for merging?
@xFrednet

xFrednet
xFrednet reviewed Apr 2, 2022
View reviewed changes

@xFrednet xFrednet left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Some minor comments and then it should be ready for merge. If you can, could you maybe also squash the intermediate commits that only added formatting or fixed something?

Comment thread clippy_lints/src/methods/err_expect.rs Resolved
Comment thread clippy_lints/src/methods/err_expect.rs Resolved
Comment thread clippy_lints/src/methods/mod.rs Resolved
@ghost

ghost commented Apr 2, 2022

Copy link
Copy Markdown
Author

How are we looking now ? 😄 @xFrednet

@ghost

ghost commented Apr 2, 2022

Copy link
Copy Markdown
Author

I may have done an oopsie while squashing, oh well.. I'll just squash it all into one commit

@ghost

ghost commented Apr 3, 2022

Copy link
Copy Markdown
Author

@xFrednet

xFrednet
xFrednet reviewed Apr 6, 2022
View reviewed changes

@xFrednet xFrednet left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This version looks good to me, I have two more comments, that should be fixed quickly, and then we can merge this 🙃

I may have done an oopsie while squashing, oh well.. I'll just squash it all into one commit

It doesn't hurt to have everything in just one commit. Thank you for squashing. You can add the requested changes as a new commit or squash them if you like 🙃

Comment thread clippy_lints/src/methods/mod.rs Resolved
Comment thread clippy_lints/src/methods/err_expect.rs Resolved
InfRandomness added 2 commits April 6, 2022 19:25
InfRandomness
Add .err().expect() lint
cebe575
infrandomness
Fix mistakes in documentation :
a7125b0 
- err() was meant to be employed instead of ok()
- wraps comment
@ghost

ghost commented Apr 6, 2022

Copy link
Copy Markdown
Author

This version looks good to me, I have two more comments, that should be fixed quickly, and then we can merge this upside_down_face

I may have done an oopsie while squashing, oh well.. I'll just squash it all into one commit

It doesn't hurt to have everything in just one commit. Thank you for squashing. You can add the requested changes as a new commit or squash them if you like upside_down_face

I have applied the changes but didn't squash this time, let me know if you need the commit description changed :)

@xFrednet

xFrednet commented Apr 6, 2022

Copy link
Copy Markdown
Contributor

Everything looks good to me, thank you for the work you put into this! I hope you also had fun 🙃

@bors r+

@bors

bors commented Apr 6, 2022

Copy link
Copy Markdown
Contributor

📌 Commit a7125b0 has been approved by xFrednet

@bors

bors commented Apr 6, 2022

Copy link
Copy Markdown
Contributor

⌛ Testing commit a7125b0 with merge 294f772...

bors added a commit that referenced this pull request Apr 6, 2022
@bors
Auto merge of #8606 - InfRandomness:err()-expect()-lint, r=xFrednet
294f772 
Add [`err_expect`] lint

[`expect_err`] lint

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

Fixes #1435

changelog:
Added a lint to detect usage of .err().expect()
@bors

bors commented Apr 6, 2022

Copy link
Copy Markdown
Contributor

💔 Test failed - checks-action_test

@ghost

ghost commented Apr 6, 2022

Copy link
Copy Markdown
Author

Oops, I edited the body of the PR, I think bors didn't like having a newline in there

@xFrednet

xFrednet commented Apr 6, 2022

Copy link
Copy Markdown
Contributor

Correct, nice quick fix. Thank you!!

@bors retry (changelog)

@bors

bors commented Apr 6, 2022

Copy link
Copy Markdown
Contributor

⌛ Testing commit a7125b0 with merge 409a936...

@bors

bors commented Apr 6, 2022

Copy link
Copy Markdown
Contributor

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

@bors
bors merged commit 409a936 into rust-lang:master Apr 6, 2022
@bors bors mentioned this pull request Apr 6, 2022
Merged
@ghost
ghost deleted the err()-expect()-lint branch April 6, 2022 18:24
This was referenced Apr 6, 2022
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

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.

ok_unwrap lint

4 participants

@rust-highfive @xFrednet @bors @camsteffen