Skip to content

Additional restrictions#4657

Merged
bors merged 4 commits intorust-lang:masterfrom
Licenser:additional-restrictions
Oct 18, 2019
Merged

Additional restrictions#4657
bors merged 4 commits intorust-lang:masterfrom
Licenser:additional-restrictions

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

Licenser commented Oct 12, 2019

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

flip1995
flip1995 reviewed Oct 13, 2019
View reviewed changes
@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

Licenser commented Oct 15, 2019

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.

@flip1995
Copy link
Member

flip1995 commented Oct 16, 2019

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

Licenser commented Oct 16, 2019

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

flip1995 commented Oct 17, 2019

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

Licenser commented Oct 17, 2019

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

Licenser commented Oct 17, 2019

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

flip1995 commented Oct 18, 2019

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

Comments