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

Fix unwrap not to lint the #[test]ing code #5842

Closed
wants to merge 1 commit into from
Closed

Fix unwrap not to lint the #[test]ing code #5842

wants to merge 1 commit into from

Conversation

chansuke
Copy link
Contributor

Fixes #5805.
changelog: none

tesuji
tesuji reviewed Jul 25, 2020
View reviewed changes
tests/ui/unwrap.rs Outdated
Comment on lines 30 to 31
with_test();
with_cfg_test();
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think you need to add these function to main.
Use https://rustc-dev-guide.rust-lang.org/tests/adding.html#revisions with compile flag for test
is better I think.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for the review and suggestion. I will fix this later.

flip1995
flip1995 requested changes Jul 26, 2020
View reviewed changes
clippy_lints/src/unwrap.rs Outdated Show resolved Hide resolved
clippy_lints/src/unwrap.rs Outdated Show resolved Hide resolved
@flip1995 flip1995 self-assigned this Jul 26, 2020
@flip1995 flip1995 added the S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) label Jul 26, 2020
@chansuke chansuke force-pushed the update_unwrap_used branch 3 times, most recently from 26cf568 to 48d223f Compare August 2, 2020 07:53
flip1995
flip1995 reviewed Aug 2, 2020
View reviewed changes
tests/ui/unwrap.rs Outdated
Comment on lines 13 to 15
// Should not get linted.
#[test]
fn with_test() {
Copy link
Member

Choose a reason for hiding this comment

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

This doesn't get linted, but not because the linting code prevents this. Code marked with #[test] attributes in an UI test file will get removed, before the test is run.

@chansuke chansuke force-pushed the update_unwrap_used branch from 48d223f to 57b3f70 Compare August 5, 2020 12:29
@bors
Copy link
Contributor

bors commented Aug 5, 2020

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

@chansuke chansuke force-pushed the update_unwrap_used branch 3 times, most recently from 10ea4c8 to f750a3f Compare August 9, 2020 00:46
@flip1995
Copy link
Member

Impl looks good to me now, but tests are missing.

@chansuke chansuke force-pushed the update_unwrap_used branch 2 times, most recently from e768946 to c8ad132 Compare August 15, 2020 13:45
@chansuke chansuke force-pushed the update_unwrap_used branch from c8ad132 to 2d07c6f Compare August 25, 2020 01:22
flip1995
flip1995 reviewed Aug 25, 2020
View reviewed changes
tests/ui/unwrap.stderr
Comment on lines +18 to +24
error: used `unwrap()` on `an Option` value
--> $DIR/unwrap.rs:20:17
|
LL | let _ = opt.unwrap();
| ^^^^^^^^^^^^
|
= help: if you don't want to handle the `None` case gracefully, consider using `expect()` to provide a better panic message
Copy link
Member

Choose a reason for hiding this comment

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

Wait, this shouldn't get linted 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I'm debugging right now but stacking

@chansuke chansuke force-pushed the update_unwrap_used branch from 2d07c6f to d38fe0f Compare October 4, 2020 07:04
@chansuke chansuke changed the title WIP: Fix unwrap not to lint the #[test]ing code Fix unwrap not to lint the #[test]ing code Oct 4, 2020
@flip1995
Copy link
Member

ping from triage @chansuke. It seems the bug with the test module wasn't fixed yet. Do you need help with debugging this?

@chansuke
Copy link
Contributor Author

@flip1995 i need help, thanks.

@flip1995
Copy link
Member

flip1995 commented Oct 10, 2020 •
edited
Loading

Uhm, the UNWRAP_USED lint is defined (and triggered) in methods/mod.rs, not in unwrap.rs. So you have to move the fix there.

@chansuke
Copy link
Contributor Author

@flip1995 Sorry, I tried to fix but stuck. I would like to close my PR.

@chansuke chansuke closed this Oct 24, 2020
@ebroto ebroto added S-inactive-closed Status: Closed due to inactivity and removed S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) labels Oct 24, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tesuji tesuji tesuji left review comments

@flip1995 flip1995 flip1995 requested changes

Assignees

@flip1995 flip1995

Labels
S-inactive-closed Status: Closed due to inactivity
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

unwrap_used lint fires for #[test]ing code
5 participants
@chansuke @bors @flip1995 @tesuji @ebroto