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

Additional restrictions #4657

Merged
merged 4 commits into from
Oct 18, 2019
Merged

Additional restrictions #4657

merged 4 commits into from
Oct 18, 2019

Conversation

Licenser
Copy link
Contributor

@Licenser Licenser commented Oct 11, 2019 •
edited by flip1995
Loading

Add restriction lints for panic!, unreachable!, todo! and .expect(...)

changelog: Add 5 new restriction lints: panic, unreachable, todo, option_expect_used, result_expect_used

@llogiq
Copy link
Contributor

llogiq commented Oct 11, 2019

Since you have panic! and unimplemented!, you probably also want to catch todo! which was recently sent on its way to stabilization.

@Licenser
Copy link
Contributor Author

yes! thanks I didn't knew that it makes total sense :D

@bors
Copy link
Contributor

bors commented Oct 15, 2019

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

@Licenser Licenser force-pushed the additional-restrictions branch from 6930427 to 53c8a08 Compare October 15, 2019 13:23
@Licenser
Copy link
Contributor Author

The failures seem to relate to something outside of the PR, and I can't run tests locally any more either because of a dependency failing, is that a known issue?

@llogiq
Copy link
Contributor

llogiq commented Oct 15, 2019

We likely need another rustup. Hang in there, we'll tell you when it's ready to rebase.

flip1995
flip1995 requested changes Oct 16, 2019
View reviewed changes
Copy link
Member

@flip1995 flip1995 left a comment

Choose a reason for hiding this comment

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

LGTM so far.

The tests need some work (mainly creating new files for the tests). Also some minor copy&paste fallouts.

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/panic_unimplemented.rs Outdated Show resolved Hide resolved
clippy_lints/src/panic_unimplemented.rs Outdated Show resolved Hide resolved
tests/ui/methods.rs Outdated Show resolved Hide resolved
tests/ui/panic_unimplemented.rs Outdated Show resolved Hide resolved
@flip1995
Copy link
Member

Rustup is done and Clippy master builds again.

@Licenser Licenser force-pushed the additional-restrictions branch from 8fd7932 to 76d0590 Compare October 16, 2019 14:32
@Licenser
Copy link
Contributor Author

Split out the tests into two now sets expect_unwrap that cover .expct("") and .unwrap("") for Result and Option - I moved one test from methods in there. And as suggested panicking_macros.

@flip1995
Copy link
Member

Tests look really good now, thanks!

If you want to, we can merge this PR already as it is right now and you can add the remaining 2 lints in another PR. Only thing left to do here, would be to squash some of the commits.

@Licenser
Copy link
Contributor Author

Ja I wanted to ask about the squashing policy anyway :D will do and rebase so we can pull them in.

@Licenser Licenser force-pushed the additional-restrictions branch from 2904e4b to 6a48a71 Compare October 17, 2019 14:55
@Licenser Licenser marked this pull request as ready for review October 17, 2019 15:37
@Licenser
Copy link
Contributor Author

Commits are squashed!

@bors
Copy link
Contributor

bors commented Oct 18, 2019

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

Heinz Gies and others added 3 commits October 18, 2019 07:35
@Licenser Licenser force-pushed the additional-restrictions branch from 6a48a71 to 06a9eca Compare October 18, 2019 05:39
@Licenser Licenser force-pushed the additional-restrictions branch from 06a9eca to 7f454d8 Compare October 18, 2019 05:41
@flip1995
Copy link
Member

Thanks, I like PRs that add 5 new lints at once 🎉

@bors r+

@bors
Copy link
Contributor

bors commented Oct 18, 2019

📌 Commit 7f454d8 has been approved by flip1995

@bors
Copy link
Contributor

bors commented Oct 18, 2019

⌛ Testing commit 7f454d8 with merge c0b2411...

bors added a commit that referenced this pull request Oct 18, 2019
@bors
Auto merge of #4657 - Licenser:additional-restrictions, r=flip1995
c0b2411 
Additional restrictions

Add restriction lints for `panic!`, `unreachable!`, `todo!` and `.expect(...)`

changelog: Add 5 new `restriction` lints: `panic`, `unreachable`, `todo`, `option_expect_used`, `result_expect_used`
@bors
Copy link
Contributor

bors commented Oct 18, 2019

☀️ Test successful - checks-travis, status-appveyor
Approved by: flip1995
Pushing c0b2411 to master...

@bors bors merged commit 7f454d8 into rust-lang:master Oct 18, 2019
This was referenced Oct 18, 2019
Merged
Merged
Closed
@Licenser Licenser mentioned this pull request Oct 18, 2019
Merged
@flip1995 flip1995 mentioned this pull request Oct 19, 2019
Closed
@flip1995 flip1995 mentioned this pull request Dec 28, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@flip1995 flip1995 flip1995 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@Licenser @llogiq @bors @flip1995