[Fix #4130] Add a new cop Style/RedundantConditional #4453

petehamilton · 2017-06-02T07:28:48Z

Returning true/false from a conditional involving a boolean conditional
is unnecessary. Instead it makes more sense to just use the boolean
expression itself.

This cop identifies and autocorrects this in ternary, if/else and
modifier form.

See #4130

Before submitting the PR make sure the following are checked:

Drenmi · 2017-06-02T08:29:19Z

lib/rubocop/cop/style/redundant_ternary.rb

+      class RedundantTernary < Cop
+        include ConfigurableEnforcedStyle
+
+        OPS = %w[== === != < > <= >= <=>].freeze


There should be a COMPARISON_METHODS constant available to you from Cop, I believe. 🙂

Ah nice, I took this from the Lint/UselessComparison cop but good to know there's a constant, I can only see it used in Style/YodaCondition but happy to follow that.

Drenmi · 2017-06-02T08:31:27Z

lib/rubocop/cop/style/redundant_ternary.rb

+        def on_if(node)
+          return unless node.ternary? && offense?(node)
+
+          add_offense(node, node.source_range, MSG)


MSG is passed automagically to #add_offense if you omit it. There are also keywords available for the second argument, giving:

add_offense(node, :expression)

Nice, thanks! Took this from Style/TernaryParentheses but will change.

From a quick look at the code, :expression results in node.loc.expression being used as the range rather than node.source_range but I'm pretty unfamiliar with this code so I'm not sure how they differ. Is there a good place to look in terms of learning more about this part of rubocop?

Drenmi · 2017-06-02T08:33:01Z

lib/rubocop/cop/style/redundant_ternary.rb

+          redundant_ternary?(node) || redundant_ternary_inverted?(node)
+        end
+
+        def autocorrect(node)


There are two #autocorrect methods right now. 😅

Drenmi · 2017-06-02T08:34:03Z

spec/rubocop/cop/style/redundant_ternary_spec.rb

+  it_behaves_like "code without offense",
+                  "x == y ? 1 : 10"
+
+  xcontext "TODO" do


We don't use code for project tracking. Leave a GitHub issue if you intend to do something later. 🙂

For sure, I was planning to fill a few of these in before merging if possible then move the rest to issues and resolve in followup PRs, this was just my working copy at the time, should have made that clearer, sorry!

bbatsov · 2017-06-02T08:58:24Z

Btw, probably this cop can easily be extended to cover if/else as well. After all in the AST both if and the ternary op are represented similarly.

petehamilton · 2017-06-02T21:15:24Z

@bbatsov @Drenmi have updated this to cover ternary and if/else cases. Would be great to get another review.

Currently I'm planning to leave the following as points for discussion in issues later:

# Generalising to any expression, not just boolean ones
Before: expression ? true : false => expression
 After: expression ? false : true => !expression

# Support negation of boolean logic
Before: x == y
 After: x != y

Before: x == y && a < b ? false : true
 After: x != y || a >=b

# Support boolean method conventions?
Before: foo? ? true : false
 After: foo?

Before: foo? ? false : true
 After: !foo?

# Support assignment of ternaries?
Before: foo = bar? ? baz : nil
 After: foo = baz if bar?

Before: foo = bar? ? nil : baz
 After: foo = baz unless bar?

petehamilton · 2017-06-02T21:55:12Z

spec/rubocop/cop/style/redundant_conditional_spec.rb

+                  'x == y ? 1 : 10'
+
+  it_behaves_like 'code with offense',
+                  "if x == y\n  true\nelse\n  false\nend",


Happy to change these to be heredocs or similar if preferred.

They would read better for sure.

Drenmi · 2017-06-03T03:49:44Z

lib/rubocop/cop/style/redundant_conditional.rb

+      #   x != y
+      class RedundantConditional < Cop
+        COMPARISON_OPERATORS = RuboCop::AST::Node::COMPARISON_OPERATORS
+                               .reject { |op| op == :! }.freeze


I think :! has been incorrectly lumped together with comparison operators in the constant in Node. It's okay to have a special case for it in your PR. I'll see if I can surgically remove it at a later point. 🙂

@petehamilton: Please see #4461. 🙂

🙌 - have updated code to reflect

bbatsov · 2017-06-03T09:07:12Z

lib/rubocop/cop/style/redundant_conditional.rb

+      class RedundantConditional < Cop
+        COMPARISON_OPERATORS = RuboCop::AST::Node::COMPARISON_OPERATORS
+
+        MSG = 'Returning true/false from a conditional is unnecessary.'.freeze


I'd check this to "This conditional expression can be replaced with its condition.". Maybe you can interpolate the condition in the message.

Done in 503bc9c. I haven't implemented interpolation as I figure we can't guarantee that the conditional will be short and a single-line string, which would presumably result in messy output?

While you're technically speaking true, most conditions are pretty short, so this might not be that big of a deal in practice.

Fair enough - happy to change.

bbatsov · 2017-06-03T09:07:26Z

lib/rubocop/cop/style/redundant_conditional.rb

+module RuboCop
+  module Cop
+    module Style
+      # This cop checks for redundant returning of true/false in conditionals


This should end with a ..

petehamilton · 2017-06-03T14:29:24Z

spec/rubocop/cop/style/redundant_conditional_spec.rb

+                    end
+                  END
+                  "x == y\n",
+                  'x == y'


@bbatsov not too happy with these specs now, they feel kind of messy with the trailing newline and then a repeat for the message. Any thoughts?

Could assume the message will be the resulting code stripped of any trailing newlines but that's not guaranteed to be true.

There's some room for improvement for sure. I don't care much about the newlines, but I think you can pass some param which you'd interpolate in the it message, so it'd be more apparent which test is testing what. Right now the output would be pretty confusing.

bbatsov · 2017-06-04T09:53:17Z

spec/rubocop/cop/style/redundant_conditional_spec.rb

+                  'x == y'
+
+  it_behaves_like 'code with offense',
+                  <<-END.strip_indent,


We typically use RUBY instead of END for ruby code snippets.

petehamilton · 2017-06-23T09:59:16Z

@bbatsov hey, sorry for taking a little while to come back to this one. Keen to get it over the line.

Right now test output looks like this with -f d (i'm inspecting the code so the newlines don't screw up the output).

> be rspec spec/rubocop/cop/style/redundant_conditional_spec.rb -f d
Run options:
  include {:focus=>true}
  exclude {:broken=>#<Proc:./spec/spec_helper.rb:39>}

All examples were filtered out; ignoring {:focus=>true}

Randomized with seed 14967

RuboCop::Cop::Style::RedundantConditional
  behaves like code with offense
    when checking "if x == y\n  false\nelse\n  true\nend\n"
      auto-corrects
      registers an offense
      claims to auto-correct
  behaves like code without offense
    when checking "if x == y\n  1\nelse\n  2\nend\n"
      does not register an offense
  behaves like code with offense
    when checking "x == y ? true : false"
      claims to auto-correct
      auto-corrects
      registers an offense
  behaves like code with offense
    when checking "if x == y\n  true\nelse\n  false\nend\n"
      auto-corrects
      registers an offense
      claims to auto-correct
  behaves like code with offense
    when checking "x == y ? false : true"
      registers an offense
      claims to auto-correct
      auto-corrects
  behaves like code without offense
    when checking "x == y ? 1 : 10"
      does not register an offense

Finished in 0.10577 seconds (files took 1.23 seconds to load)
14 examples, 0 failures

Randomized with seed 14967

What are you suggesting as an alternative? Providing a description of each code snippet instead of the code inline?

For context, I was just following this as an example but happy to change!

bbatsov · 2017-06-30T14:13:56Z

Right now test output looks like this with -f d (i'm inspecting the code so the newlines don't screw up the output).

The output is good enough for now. Just squash the commits together, rebase on top of master and get the build to pass.

petehamilton · 2017-06-30T14:15:07Z

@bbatsov on it.

dobrynin · 2017-08-08T18:54:12Z

Hi, I noticed this issue while using Rubocop as well. What's the status of the PR?

petehamilton · 2017-08-09T14:36:52Z

@dobrynin - my bad, I had it nearly ready to go and then started getting test failures after a rebase and totally dropped the ball.

Turned out they were coming from the change in test inspection methods introduced in #4510. All fixed up and hopefully tests should go green now, at which point I think this should be good to merge.

Returning true/false from a conditional involving a boolean conditional is unnecessary. Instead it makes more sense to just use the boolean expression itself. This cop identifies and autocorrects this in ternary and if/else statements.

petehamilton · 2017-08-09T16:04:48Z

@bbatsov @Drenmi all 👍 ? Think this should be good to merge now.

petehamilton mentioned this pull request Jun 2, 2017

Introduce cop for redundant ternary condition #4130

Closed

Drenmi reviewed Jun 2, 2017

View reviewed changes

petehamilton changed the title ~~Add cop to check for redundant ternary conditions~~ [Fix #4130] Introduce cop for redundant conditional Jun 2, 2017

petehamilton changed the title ~~[Fix #4130] Introduce cop for redundant conditional~~ [Fix #4130] Introduce cop for redundant ternary Jun 2, 2017

petehamilton force-pushed the implement-redundant-ternary branch 2 times, most recently from da5373f to 7f3749c Compare June 2, 2017 21:07

petehamilton changed the title ~~[Fix #4130] Introduce cop for redundant ternary~~ [Fix #4130] Introduce cop for redundant conditions Jun 2, 2017

petehamilton force-pushed the implement-redundant-ternary branch 2 times, most recently from 5fd2463 to f9218b4 Compare June 2, 2017 21:13

petehamilton changed the title ~~[Fix #4130] Introduce cop for redundant conditions~~ [Fix #4130] Add a new cop Style/RedundantConditional Jun 2, 2017

petehamilton force-pushed the implement-redundant-ternary branch 5 times, most recently from a0fbdc8 to 369dfae Compare June 2, 2017 21:54

petehamilton commented Jun 2, 2017

View reviewed changes

petehamilton force-pushed the implement-redundant-ternary branch from 369dfae to 1c16ee8 Compare June 2, 2017 21:57

Drenmi reviewed Jun 3, 2017

View reviewed changes

petehamilton force-pushed the implement-redundant-ternary branch 2 times, most recently from f993e7e to 9113104 Compare June 3, 2017 08:02

bbatsov reviewed Jun 3, 2017

View reviewed changes

petehamilton force-pushed the implement-redundant-ternary branch 3 times, most recently from ea0bf94 to e8358cd Compare June 3, 2017 11:23

petehamilton commented Jun 3, 2017

View reviewed changes

bbatsov reviewed Jun 4, 2017

View reviewed changes

petehamilton force-pushed the implement-redundant-ternary branch from 68cfd52 to 2667123 Compare June 23, 2017 10:01

petehamilton force-pushed the implement-redundant-ternary branch from 2667123 to 830d911 Compare June 30, 2017 14:18

petehamilton force-pushed the implement-redundant-ternary branch from 830d911 to a63a957 Compare August 9, 2017 14:32

petehamilton force-pushed the implement-redundant-ternary branch from a63a957 to c3e488b Compare August 9, 2017 14:38

Merge branch 'master' into implement-redundant-ternary

f991541

bbatsov merged commit b6303de into rubocop:master Aug 21, 2017

petehamilton deleted the implement-redundant-ternary branch August 21, 2017 08:41

mpotter mentioned this pull request Oct 21, 2017

Upgrade to rubocop 0.51.0 ableco/ablecop#55

Merged

5 tasks

dependabot bot mentioned this pull request Mar 16, 2021

Bump rubocop from 0.47.1 to 0.50.0 LemonSpike/Xcodeproj#4

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Fix #4130] Add a new cop Style/RedundantConditional #4453

[Fix #4130] Add a new cop Style/RedundantConditional #4453

petehamilton commented Jun 2, 2017 •

edited

Loading

Drenmi Jun 2, 2017

petehamilton Jun 2, 2017 •

edited

Loading

Drenmi Jun 2, 2017

petehamilton Jun 2, 2017 •

edited

Loading

Drenmi Jun 2, 2017

petehamilton Jun 2, 2017

Drenmi Jun 2, 2017

petehamilton Jun 2, 2017

bbatsov commented Jun 2, 2017

petehamilton commented Jun 2, 2017 •

edited

Loading

petehamilton Jun 2, 2017

bbatsov Jun 3, 2017

Drenmi Jun 3, 2017

Drenmi Jun 3, 2017

petehamilton Jun 3, 2017 •

edited

Loading

bbatsov Jun 3, 2017

petehamilton Jun 3, 2017 •

edited

Loading

bbatsov Jun 3, 2017

petehamilton Jun 3, 2017

bbatsov Jun 3, 2017

petehamilton Jun 3, 2017

petehamilton Jun 3, 2017 •

edited

Loading

bbatsov Jun 4, 2017

bbatsov Jun 4, 2017

petehamilton Jun 23, 2017

petehamilton commented Jun 23, 2017 •

edited

Loading

bbatsov commented Jun 30, 2017

petehamilton commented Jun 30, 2017

dobrynin commented Aug 8, 2017

petehamilton commented Aug 9, 2017 •

edited

Loading

petehamilton commented Aug 9, 2017 •

edited

Loading

[Fix #4130] Add a new cop Style/RedundantConditional #4453

[Fix #4130] Add a new cop Style/RedundantConditional #4453

Conversation

petehamilton commented Jun 2, 2017 • edited Loading

Choose a reason for hiding this comment

petehamilton Jun 2, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

petehamilton Jun 2, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bbatsov commented Jun 2, 2017

petehamilton commented Jun 2, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

petehamilton Jun 3, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

petehamilton Jun 3, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

petehamilton Jun 3, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

petehamilton commented Jun 23, 2017 • edited Loading

bbatsov commented Jun 30, 2017

petehamilton commented Jun 30, 2017

dobrynin commented Aug 8, 2017

petehamilton commented Aug 9, 2017 • edited Loading

petehamilton commented Aug 9, 2017 • edited Loading

petehamilton commented Jun 2, 2017 •

edited

Loading

petehamilton Jun 2, 2017 •

edited

Loading

petehamilton Jun 2, 2017 •

edited

Loading

petehamilton commented Jun 2, 2017 •

edited

Loading

petehamilton Jun 3, 2017 •

edited

Loading

petehamilton Jun 3, 2017 •

edited

Loading

petehamilton Jun 3, 2017 •

edited

Loading

petehamilton commented Jun 23, 2017 •

edited

Loading

petehamilton commented Aug 9, 2017 •

edited

Loading

petehamilton commented Aug 9, 2017 •

edited

Loading