Skip to content

Commit

Permalink
Add new Style/SoleNestedConditional cop
Browse files Browse the repository at this point in the history
  • Loading branch information
fatkodima authored and bbatsov committed Aug 22, 2020
1 parent 265360c commit 4727abf
Show file tree
Hide file tree
Showing 19 changed files with 332 additions and 47 deletions.
1 change: 1 addition & 0 deletions CHANGELOG.md
Expand Up @@ -5,6 +5,7 @@
### New features

* [#8451](https://github.com/rubocop-hq/rubocop/issues/8451): Add new `Style/RedundantSelfAssignment` cop. ([@fatkodima][])
* [#8390](https://github.com/rubocop-hq/rubocop/pull/8390): Add new `Style/SoleNestedConditional` cop. ([@fatkodima][])
* [#8562](https://github.com/rubocop-hq/rubocop/pull/8562): Add new `Style/KeywordParametersOrder` cop. ([@fatkodima][])
* [#8381](https://github.com/rubocop-hq/rubocop/pull/8381): Add new `Style/ClassMethodsDefinitions` cop. ([@fatkodima][])
* [#8474](https://github.com/rubocop-hq/rubocop/pull/8474): Add new `Lint/DuplicateRequire` cop. ([@fatkodima][])
Expand Down
8 changes: 8 additions & 0 deletions config/default.yml
Expand Up @@ -4030,6 +4030,14 @@ Style/SlicingWithRange:
VersionAdded: '0.83'
Safe: false

Style/SoleNestedConditional:
Description: >-
Finds sole nested conditional nodes
which can be merged into outer conditional node.
Enabled: pending
VersionAdded: '0.89'
AllowModifier: false

Style/SpecialGlobalVars:
Description: 'Avoid Perl-style global variables.'
StyleGuide: '#no-cryptic-perlisms'
Expand Down
1 change: 1 addition & 0 deletions docs/modules/ROOT/pages/cops.adoc
Expand Up @@ -486,6 +486,7 @@ In the following section you find all available cops:
* xref:cops_style.adoc#stylesinglelineblockparams[Style/SingleLineBlockParams]
* xref:cops_style.adoc#stylesinglelinemethods[Style/SingleLineMethods]
* xref:cops_style.adoc#styleslicingwithrange[Style/SlicingWithRange]
* xref:cops_style.adoc#stylesolenestedconditional[Style/SoleNestedConditional]
* xref:cops_style.adoc#stylespecialglobalvars[Style/SpecialGlobalVars]
* xref:cops_style.adoc#stylestabbylambdaparentheses[Style/StabbyLambdaParentheses]
* xref:cops_style.adoc#stylestderrputs[Style/StderrPuts]
Expand Down
63 changes: 63 additions & 0 deletions docs/modules/ROOT/pages/cops_style.adoc
Expand Up @@ -9077,6 +9077,69 @@ items[1..-1]
items[1..]
----

== Style/SoleNestedConditional

|===
| Enabled by default | Safe | Supports autocorrection | VersionAdded | VersionChanged

| Pending
| Yes
| No
| 0.89
| -
|===

If the branch of a conditional consists solely of a conditional node,
its conditions can be combined with the conditions of the outer branch.
This helps to keep the nesting level from getting too deep.

=== Examples

[source,ruby]
----
# bad
if condition_a
if condition_b
do_something
end
end
# good
if condition_a && condition_b
do_something
end
----

==== AllowModifier: false (default)

[source,ruby]
----
# bad
if condition_a
do_something if condition_b
end
----

==== AllowModifier: true

[source,ruby]
----
# good
if condition_a
do_something if condition_b
end
----

=== Configurable attributes

|===
| Name | Default value | Configurable values

| AllowModifier
| `false`
| Boolean
|===

== Style/SpecialGlobalVars

|===
Expand Down
1 change: 1 addition & 0 deletions lib/rubocop.rb
Expand Up @@ -458,6 +458,7 @@
require_relative 'rubocop/cop/style/redundant_fetch_block'
require_relative 'rubocop/cop/style/redundant_file_extension_in_require'
require_relative 'rubocop/cop/style/redundant_self_assignment'
require_relative 'rubocop/cop/style/sole_nested_conditional'
require_relative 'rubocop/cop/style/method_call_with_args_parentheses/omit_parentheses'
require_relative 'rubocop/cop/style/method_call_with_args_parentheses/require_parentheses'
require_relative 'rubocop/cop/style/method_called_on_do_end_block'
Expand Down
4 changes: 2 additions & 2 deletions lib/rubocop/cop/correctors/alignment_corrector.rb
Expand Up @@ -47,8 +47,8 @@ def autocorrect_line(corrector, line_begin_pos, expr, column_delta,
# string literals
return if taboo_ranges.any? { |t| within?(range, t) }

if column_delta.positive?
corrector.insert_before(range, ' ' * column_delta) unless range.resize(1).source == "\n"
if column_delta.positive? && range.resize(1).source != "\n"
corrector.insert_before(range, ' ' * column_delta)
elsif /\A[ \t]+\z/.match?(range.source)
remove(range, corrector)
end
Expand Down
5 changes: 2 additions & 3 deletions lib/rubocop/cop/layout/first_method_argument_line_break.rb
Expand Up @@ -35,9 +35,8 @@ def on_send(node)
#
# ...then each key/value pair is treated as a method 'argument'
# when determining where line breaks should appear.
if (last_arg = args.last)
args.concat(args.pop.children) if last_arg.hash_type? && !last_arg.braces?
end
last_arg = args.last
args.concat(args.pop.children) if last_arg&.hash_type? && !last_arg&.braces?

check_method_line_break(node, args)
end
Expand Down
Expand Up @@ -36,9 +36,8 @@ def on_send(node)
#
# ...then each key/value pair is treated as a method 'argument'
# when determining where line breaks should appear.
if (last_arg = args.last)
args = args[0...-1] + last_arg.children if last_arg.hash_type? && !last_arg.braces?
end
last_arg = args.last
args = args[0...-1] + last_arg.children if last_arg&.hash_type? && !last_arg&.braces?

check_line_breaks(node, args)
end
Expand Down
4 changes: 2 additions & 2 deletions lib/rubocop/cop/layout/space_around_operators.rb
Expand Up @@ -176,8 +176,8 @@ def enclose_operator_with_space(corrector, range)
# If `ForceEqualSignAlignment` is true, `Layout/ExtraSpacing` cop
# inserts spaces before operator. If `Layout/SpaceAroundOperators` cop
# inserts a space, it collides and raises the infinite loop error.
if force_equal_sign_alignment?
corrector.insert_after(range, ' ') unless operator.end_with?(' ')
if force_equal_sign_alignment? && !operator.end_with?(' ')
corrector.insert_after(range, ' ')
else
corrector.replace(range, " #{operator.strip} ")
end
Expand Down
4 changes: 2 additions & 2 deletions lib/rubocop/cop/lint/missing_super.rb
Expand Up @@ -56,8 +56,8 @@ class MissingSuper < Base
def on_def(node)
return unless offender?(node)

if node.method?(:initialize)
add_offense(node, message: CONSTRUCTOR_MSG) if inside_class_with_stateful_parent?(node)
if node.method?(:initialize) && inside_class_with_stateful_parent?(node)
add_offense(node, message: CONSTRUCTOR_MSG)
elsif callback_method_def?(node)
add_offense(node, message: CALLBACK_MSG)
end
Expand Down
12 changes: 5 additions & 7 deletions lib/rubocop/cop/mixin/check_line_breakable.rb
Expand Up @@ -130,10 +130,9 @@ def contained_by_breakable_collection_on_same_line?(node)

def contained_by_multiline_collection_that_could_be_broken_up?(node)
node.each_ancestor.find do |ancestor|
if ancestor.hash_type? || ancestor.array_type?
if breakable_collection?(ancestor, ancestor.children)
return children_could_be_broken_up?(ancestor.children)
end
if (ancestor.hash_type? || ancestor.array_type?) &&
breakable_collection?(ancestor, ancestor.children)
return children_could_be_broken_up?(ancestor.children)
end

next unless ancestor.send_type?
Expand Down Expand Up @@ -170,9 +169,8 @@ def process_args(args)
#
# ...then each key/value pair is treated as a method 'argument'
# when determining where line breaks should appear.
if (last_arg = args.last)
args = args[0...-1] + last_arg.children if last_arg.hash_type? && !last_arg.braces?
end
last_arg = args.last
args = args[0...-1] + last_arg.children if last_arg&.hash_type? && !last_arg&.braces?
args
end

Expand Down
11 changes: 6 additions & 5 deletions lib/rubocop/cop/style/hash_syntax.rb
Expand Up @@ -140,11 +140,12 @@ def word_symbol_pair?(pair)
def acceptable_19_syntax_symbol?(sym_name)
sym_name.sub!(/\A:/, '')

if cop_config['PreferHashRocketsForNonAlnumEndingSymbols']
# Prefer { :production? => false } over { production?: false } and
# similarly for other non-alnum final characters (except quotes,
# to prefer { "x y": 1 } over { :"x y" => 1 }).
return false unless /[\p{Alnum}"']\z/.match?(sym_name)
if cop_config['PreferHashRocketsForNonAlnumEndingSymbols'] &&
# Prefer { :production? => false } over { production?: false } and
# similarly for other non-alnum final characters (except quotes,
# to prefer { "x y": 1 } over { :"x y" => 1 }).
!/[\p{Alnum}"']\z/.match?(sym_name)
return false
end

# Most hash keys can be matched against a simple regex.
Expand Down
4 changes: 2 additions & 2 deletions lib/rubocop/cop/style/if_unless_modifier.rb
Expand Up @@ -43,8 +43,8 @@ class IfUnlessModifier < Base
'Modifier form of `%<keyword>s` makes the line too long.'

def on_if(node)
msg = if single_line_as_modifier?(node)
MSG_USE_MODIFIER unless named_capture_in_condition?(node)
msg = if single_line_as_modifier?(node) && !named_capture_in_condition?(node)
MSG_USE_MODIFIER
elsif too_long_due_to_modifier?(node)
MSG_USE_NORMAL
end
Expand Down
5 changes: 2 additions & 3 deletions lib/rubocop/cop/style/redundant_parentheses.rb
Expand Up @@ -112,9 +112,8 @@ def check_unary(begin_node, node)

node = node.children.first while suspect_unary?(node)

if node.send_type?
return unless method_call_with_redundant_parentheses?(node)
end
return if node.send_type? &&
!method_call_with_redundant_parentheses?(node)

offense(begin_node, 'an unary operation')
end
Expand Down
66 changes: 66 additions & 0 deletions lib/rubocop/cop/style/sole_nested_conditional.rb
@@ -0,0 +1,66 @@
# frozen_string_literal: true

module RuboCop
module Cop
module Style
# If the branch of a conditional consists solely of a conditional node,
# its conditions can be combined with the conditions of the outer branch.
# This helps to keep the nesting level from getting too deep.
#
# @example
# # bad
# if condition_a
# if condition_b
# do_something
# end
# end
#
# # good
# if condition_a && condition_b
# do_something
# end
#
# @example AllowModifier: false (default)
# # bad
# if condition_a
# do_something if condition_b
# end
#
# @example AllowModifier: true
# # good
# if condition_a
# do_something if condition_b
# end
#
class SoleNestedConditional < Base
MSG = 'Consider merging nested conditions into '\
'outer `%<conditional_type>s` conditions.'

def on_if(node)
return if node.ternary? || node.else? || node.elsif?

branch = node.if_branch
return unless offending_branch?(branch)

message = format(MSG, conditional_type: node.keyword)
add_offense(branch.loc.keyword, message: message)
end

private

def offending_branch?(branch)
return false unless branch

branch.if_type? &&
!branch.else? &&
!branch.ternary? &&
!(branch.modifier_form? && allow_modifier?)
end

def allow_modifier?
cop_config['AllowModifier']
end
end
end
end
end
8 changes: 3 additions & 5 deletions lib/rubocop/cop/style/trailing_underscore_variable.rb
Expand Up @@ -68,11 +68,9 @@ def find_first_possible_offense(variables)

var, = *variable
var, = *var
if allow_named_underscore_variables
break offense unless var == :_
else
break offense unless var.to_s.start_with?(UNDERSCORE)
end

break offense if (allow_named_underscore_variables && var != :_) ||
!var.to_s.start_with?(UNDERSCORE)

variable
end
Expand Down
10 changes: 1 addition & 9 deletions lib/rubocop/cop/style/yoda_condition.rb
Expand Up @@ -154,15 +154,7 @@ def program_name?(name)
def interpolation?(node)
return true if node.dstr_type?

# TODO: Use `RegexpNode#interpolation?` when the following is released.
# https://github.com/rubocop-hq/rubocop-ast/pull/18
if node.regexp_type?
return true if node.children.any? do |child|
child.respond_to?(:begin_type?) && child.begin_type?
end
end

false
node.regexp_type? && node.interpolation?
end
end
end
Expand Down
8 changes: 4 additions & 4 deletions lib/rubocop/rspec/expect_offense.rb
Expand Up @@ -241,10 +241,10 @@ def ==(other)
def match_annotations?(other)
annotations.zip(other.annotations) do |(_actual_line, actual_annotation),
(_expected_line, expected_annotation)|
if expected_annotation&.end_with?(ABBREV)
if actual_annotation.start_with?(expected_annotation[0...-ABBREV.length])
expected_annotation.replace(actual_annotation)
end
if expected_annotation&.end_with?(ABBREV) &&
actual_annotation.start_with?(expected_annotation[0...-ABBREV.length])

expected_annotation.replace(actual_annotation)
end
end

Expand Down

0 comments on commit 4727abf

Please sign in to comment.