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

Add new Style/IfWithBooleanLiteralBranches cop #9396

Conversation

koic
Copy link
Member

@koic koic commented Jan 20, 2021

This PR adds new Style/IfWithBooleanLiteralBranches cop that checks for redundant if with boolean literal branches.

It checks only conditions to return boolean value (true or false) for safe detection.
The conditions to be checked are comparison methods, predicate methods, and double negative.

# bad
if foo == bar
  true
else
  false
end

# bad
foo == bar ? true : false

# good
foo == bar

And this PR auto-corrected the following offense by the new cop.

% bundle exec rake
(snip)

Offenses:

lib/rubocop/cop/style/ternary_parentheses.rb:123:47: C: [Correctable]
Style/IfWithBooleanLiteralBranches: Remove redundant ternary operator
with boolean literal branches.
            non_complex_expression?(condition) ? false : true
                                              ^^^^^^^^^^^^^^^

1246 files inspected, 1 offense detected, 1 offense auto-correctable
RuboCop failed!

Before submitting the PR make sure the following are checked:

  • The PR relates to only one subject with a clear title and description in grammatically correct, complete sentences.
  • Wrote good commit messages.
  • Commit message starts with [Fix #issue-number] (if the related issue exists).
  • Feature branch is up-to-date with master (if not - rebase it).
  • Squashed related commits together.
  • Added tests.
  • Ran bundle exec rake default. It executes all tests and runs RuboCop on its own code.
  • Added an entry (file) to the changelog folder named {change_type}_{change_description}.md if the new code introduces user-observable changes. See changelog entry format for details.

@koic koic force-pushed the add_new_style_if_with_boolean_literal_branches_cop branch 2 times, most recently from a095465 to fb97847 Compare January 21, 2021 13:37
Copy link
Contributor

@marcandre marcandre left a comment

Choose a reason for hiding this comment

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

That's a great cop imo

This PR adds new `Style/IfWithBooleanLiteralBranches` cop that checks
for redundant `if` with boolean literal branches.

It checks only conditions to return boolean value (`true` or `false`)
for safe detection.
The conditions to be checked are comparison methods, predicate methods,
and double negative.

```ruby
# bad
if foo == bar
  true
else
  false
end

# bad
foo == bar ? true : false

# good
foo == bar
```

And this PR auto-corrected the following offense by the new cop.

```console
% bundle exec rake
(snip)

Offenses:

lib/rubocop/cop/style/ternary_parentheses.rb:123:47: C: [Correctable]
Style/IfWithBooleanLiteralBranches: Remove redundant ternary operator
with boolean literal branches.
            non_complex_expression?(condition) ? false : true
                                              ^^^^^^^^^^^^^^^

1246 files inspected, 1 offense detected, 1 offense auto-correctable
RuboCop failed!
```
@koic koic force-pushed the add_new_style_if_with_boolean_literal_branches_cop branch from fb97847 to b839973 Compare January 22, 2021 03:11

it 'does not register an offense when using `if foo? || bar || baz?` with boolean literal branches' do
expect_no_offenses(<<~RUBY)
if foo? || bar || baz?
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do you prefer not simplifying this one?

Copy link
Member Author

Choose a reason for hiding this comment

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

This is because the return value changes from true to "hi" if bar is not a boolean value. Example:

def foo?
  false
end

def bar
  "hi"
end

def baz?
  false
end

foo? || bar || baz? ? true : false #=> true
foo? || bar || baz? #=> "hi"

So this aims for safe detection, which keeps the return value as a boolean value.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, got it. Thanks @koic

@bbatsov bbatsov merged commit fdf25f2 into rubocop:master Jan 26, 2021
@bbatsov
Copy link
Collaborator

bbatsov commented Jan 26, 2021

Great addition indeed! Technically speaking we can extend this to cover case as well, but I guess it's quite uncommon to write such code.

@koic koic deleted the add_new_style_if_with_boolean_literal_branches_cop branch January 26, 2021 10:23
jmkoni pushed a commit to standardrb/standard that referenced this pull request May 3, 2021
* Update rubocop from 1.12.1 to [1.13.0](https://github.com/rubocop-hq/rubocop/releases/tag/v1.13.0)
* Update rubocop-performance from 1.9.2 to [1.11.1](https://github.com/rubocop-hq/rubocop-performance/releases/tag/v1.11.1)
* Enabled the following rules:
  * [`Performance/RedundantSplitRegexpArgument`](rubocop/rubocop-performance#190)
  * [`Style/IfWithBooleanLiteralBranches`](rubocop/rubocop#9396)
  * [`Lint/TripleQuotes`](rubocop/rubocop#9402)
  * [`Lint/SymbolConversion`](rubocop/rubocop#9362)
  * [`Lint/OrAssignmentToConstant`](rubocop/rubocop#9363)
  * [`Lint/NumberedParameterAssignment`](rubocop/rubocop#9326)
  * [`Style/HashConversion`](rubocop/rubocop#9478)
  * [`Gemspec/DateAssignment`](rubocop/rubocop#9496)
  * [`Style/StringChars`](rubocop/rubocop#9615)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants