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

Remove `except` in suspicious_else_formatting #3960

Merged
merged 3 commits into from Apr 14, 2019

Conversation

Projects
None yet
4 participants
@phansch
Copy link
Collaborator

commented Apr 14, 2019

96c34e8 contains the fix:

This was causing two different ICEs in #3741. The first was fixed in #3925.

The second one is fixed with this commit: We just don't expect anymore.
If the snippet doesn't contain an else, we stop emitting the lint because
it's not a suspiciously formatted else anyway.

Unfortunately I wasn't able to provide a minimal test case, but I think it's
fine since it's just ignoring the None case now.

And ad27e3f cleans up the lint code to use if_chain.

Fixes #3741 once more.

phansch added some commits Apr 14, 2019

Remove `except` in suspicious_else_formatting
This was causing two different ICEs in #3741.
The first was fixed in #3925.

The second one is fixed with this commit: We just don't `expect`
anymore. If the snippet doesn't contain an `else`, we stop emitting the
lint because it's not a suspiciously formatted else anyway.
let else_pos = else_snippet.find("else").expect("there must be a `else` here");
// this will be a span from the closing ‘}’ of the “then” block (excluding) to
// the
// “if” of the “else if” block (excluding)

This comment has been minimized.

Copy link
@flip1995

flip1995 Apr 14, 2019

Collaborator

NIT: formatting

Is it possible to add a test for this ICE? Otherwise r=me with formatting fixed.

This comment has been minimized.

Copy link
@phansch

phansch Apr 14, 2019

Author Collaborator

nit fixed =)

I tried since yesterday but couldn't come up with a test case without external dependencies on relm and gtk. I think it's fine to merge it in this case since the ICE was triggered by an explicit expect in our code and it would only regress if we were to add it back.

@phansch phansch force-pushed the phansch:fix_except branch from 0db8b6e to 289a9af Apr 14, 2019

@flip1995

This comment has been minimized.

Copy link
Collaborator

commented Apr 14, 2019

@bors r+

@bors

This comment has been minimized.

Copy link
Contributor

commented Apr 14, 2019

📌 Commit 289a9af has been approved by flip1995

@bors

This comment has been minimized.

Copy link
Contributor

commented Apr 14, 2019

⌛️ Testing commit 289a9af with merge 6505794...

bors added a commit that referenced this pull request Apr 14, 2019

Auto merge of #3960 - phansch:fix_except, r=flip1995
Remove `except` in suspicious_else_formatting

96c34e8 contains the fix:

This was causing two different ICEs in #3741. The first was fixed in #3925.

The second one is fixed with this commit: We just don't `expect` anymore.
If the snippet doesn't contain an `else`, we stop emitting the lint because
it's not a suspiciously formatted else anyway.

Unfortunately I wasn't able to provide a minimal test case, but I think it's
fine since it's just ignoring the `None` case now.

And ad27e3f cleans up the lint code to use `if_chain`.

Fixes #3741 once more.
@hcpl

This comment has been minimized.

Copy link

commented Apr 14, 2019

Just a small nit, but the PR and the branch name use "except" instead of "expect". 😆

@bors

This comment has been minimized.

Copy link
Contributor

commented Apr 14, 2019

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

@bors bors merged commit 289a9af into rust-lang:master Apr 14, 2019

3 checks passed

Travis CI - Pull Request Build Passed
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
homu Test successful
Details

@phansch phansch deleted the phansch:fix_except branch Apr 18, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.