Update Style/RedundantCondition cop to detect conditional expressions where the true branch is true and suggest replacing them with a logical OR

[datpmt]

Checks for ternary expressions that use true as the true branch and suggest replacing them with a logical OR (||) expression.
For example, a ternary like condition ? true : variable is considered redundant because it can be simplified to condition || variable. The cop promotes the use of more concise and readable code by encouraging the use of logical OR in such cases, improving both clarity and maintainability.

#   # bad
#   a.nil? ? true : a
#
#   # good
#   a.nil? || a
#
#   # bad
#   if a.nil?
#     true
#   else
#     a
#   end
#
#   # good
#   a.nil? || a

---

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]

Actually, this is the responsibility of Style/RedundantCondition cop. Instead of creating a new cop, please consider extending Style/RedundantCondition cop.

[datpmt]

@koic I’ve extended the Style/RedundantCondition cop as suggested. The PR is now updated. Please let me know if anything else needs adjustment.

[koic]

Suggested change

	# if a.nil?
	# true
	# else
	# a
	# end
	# if a.nil?
	# true
	# else
	# a
	# end

[koic]

It can likely be simplified as follows.

Suggested change

	_condition, if_branch, else_branch = *node # rubocop:disable InternalAffairs/NodeDestructuring
	return false unless if_branch && else_branch
	if_branch.true_type? && !else_branch.true_type?
	node.if_branch&.true_type? && node.else_branch && !node.else_branch.true_type?

[koic]

This text belongs in it, not context. Can you update the tests added in this PR by referring to the existing tests?

[koic]

Can you add tests for cases where each branch does not exist.

      expect_no_offenses(<<~RUBY)
        if a.zero?
          true
        else
        end
      RUBY

and

      expect_no_offenses(<<~RUBY)
        if a.zero?
        else
          a
        end
      RUBY

[datpmt]

thanks @koic, I updated the PR

[bbatsov]

ternary -> conditional

[datpmt]

@bbatsov please help me review the PR, thank you

[byroot]

This cop started firing on Rails master (our fault rubocop wasn't locked in Gemfile) and I must say I'm a bit puzzled by some of its suggestions, maybe the cop isn't quite restrictive enough:

e.g.

activerecord/lib/active_record/suppressor.rb:56:44: C: [Correctable] Style/RedundantCondition: Use double pipes || instead.
      Suppressor.registry[self.class.name] ? true : super
                                           ^^^^^^^^

It I use the auto-corrector it changes it for:

     def save(**) # :nodoc:
-      Suppressor.registry[self.class.name] ? true : super
+      Suppressor.registry[self.class.name] || super
     end

Which isn't equivalent, because it used to return true and now might return all sorts of "truthy" object. This is often fine, but may very well not be.

[datpmt]

@byroot thank you,

I realize that condition must be nil or true or false. I missed a step to check this. Please let me know if I missed something.

[Earlopain]

It seems fine for the condition to be a predicate method without safe navigation. The cases you mentionend are already handled via Lint/LiteralAsCondition

[bbatsov]

@byroot Can you please file a ticket about this? We'll address it soon.

[byroot]

Ref: #13913