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

collapsible_if: split collapsible_else_if into its own lint so we can enable/disable it particularly #6544

Merged
merged 1 commit into from
Jan 4, 2021

Conversation

matthiaskrgr
Copy link
Member

This splits up clippy::collapsible_if into collapsible_if for
if x {
if y { }
}
=>
if x && y { }

and collapsible_else_if for

if x {
} else {
if y { }
}

=>
if x {

} else if y {

}

so that we can lint for only the latter but not the first if we desire.

changelog: collapsible_if: split up linting for if x {} else { if y {} } into collapsible_else_if lint

@rust-highfive
Copy link

r? @flip1995

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Jan 4, 2021
clippy_lints/src/collapsible_if.rs Outdated Show resolved Hide resolved
Comment on lines +85 to 86
pub COLLAPSIBLE_ELSE_IF,
style,
Copy link
Member

Choose a reason for hiding this comment

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

Having the split out lint in the style category will result in people having to allow this lint again in their code, if they don't like it.

I guess the argument that this is fine, is that this case of the lint is easier to fix than the other case?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, hopefully it won't be too annoying. :/
I also think that the if {} else { if {} } warning is actually more useful than the if { if {} } warning.

Copy link
Member

Choose a reason for hiding this comment

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

Looking at your Rust PR, where you linked to this, it seems Joshua also agrees. So I'm ok with this.

@flip1995 flip1995 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 Jan 4, 2021
@matthiaskrgr matthiaskrgr added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties and removed S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) labels Jan 4, 2021
style,
"`if`s that can be collapsed (e.g., `if x { if y { ... } }` and `else { if x { ... } }`)"
"nested `else`-`if` expressions that can be collapsed (e.g., `else { if x { ... } }`)"
}

declare_lint_pass!(CollapsibleIf => [COLLAPSIBLE_IF]);
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
declare_lint_pass!(CollapsibleIf => [COLLAPSIBLE_IF]);
declare_lint_pass!(CollapsibleIf => [COLLAPSIBLE_IF, COLLAPSIBLE_ELSE_IF]);

@flip1995
Copy link
Member

flip1995 commented Jan 4, 2021

r=me with CI passing.

… enable/disable it particularly

This splits up clippy::collapsible_if into collapsible_if for
if x {
  if y { }
}
=>
if x && y { }

and collapsible_else_if for

if x {
} else {
 if y { }
}

=>
if x {

} else if y {

}

so that we can lint for only the latter but not the first if we desire.

changelog: collapsible_if: split up linting for if x {} else { if y {} } into collapsible_else_if lint
@matthiaskrgr
Copy link
Member Author

@bors r=flip1995

@bors
Copy link
Collaborator

bors commented Jan 4, 2021

📌 Commit 6dcec6a has been approved by flip1995

@bors
Copy link
Collaborator

bors commented Jan 4, 2021

⌛ Testing commit 6dcec6a with merge dd1929e...

@bors
Copy link
Collaborator

bors commented Jan 4, 2021

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: flip1995
Pushing dd1929e to master...

@bors bors merged commit dd1929e into rust-lang:master Jan 4, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants