From 4727abfa88907bdf77d1b40cf58a9d9680cd286e Mon Sep 17 00:00:00 2001 From: fatkodima Date: Fri, 24 Jul 2020 01:57:53 +0300 Subject: [PATCH] Add new `Style/SoleNestedConditional` cop --- CHANGELOG.md | 1 + config/default.yml | 8 + docs/modules/ROOT/pages/cops.adoc | 1 + docs/modules/ROOT/pages/cops_style.adoc | 63 +++++++ lib/rubocop.rb | 1 + .../cop/correctors/alignment_corrector.rb | 4 +- .../first_method_argument_line_break.rb | 5 +- .../multiline_method_argument_line_breaks.rb | 5 +- .../cop/layout/space_around_operators.rb | 4 +- lib/rubocop/cop/lint/missing_super.rb | 4 +- lib/rubocop/cop/mixin/check_line_breakable.rb | 12 +- lib/rubocop/cop/style/hash_syntax.rb | 11 +- lib/rubocop/cop/style/if_unless_modifier.rb | 4 +- .../cop/style/redundant_parentheses.rb | 5 +- .../cop/style/sole_nested_conditional.rb | 66 ++++++++ .../cop/style/trailing_underscore_variable.rb | 8 +- lib/rubocop/cop/style/yoda_condition.rb | 10 +- lib/rubocop/rspec/expect_offense.rb | 8 +- .../cop/style/sole_nested_conditional_spec.rb | 159 ++++++++++++++++++ 19 files changed, 332 insertions(+), 47 deletions(-) create mode 100644 lib/rubocop/cop/style/sole_nested_conditional.rb create mode 100644 spec/rubocop/cop/style/sole_nested_conditional_spec.rb diff --git a/CHANGELOG.md b/CHANGELOG.md index 218c8a786ff..8f7b6a7dbd3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -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][]) diff --git a/config/default.yml b/config/default.yml index 46a5dd5e0ae..06c22411915 100644 --- a/config/default.yml +++ b/config/default.yml @@ -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' diff --git a/docs/modules/ROOT/pages/cops.adoc b/docs/modules/ROOT/pages/cops.adoc index 5738dfbad31..e78da508f88 100644 --- a/docs/modules/ROOT/pages/cops.adoc +++ b/docs/modules/ROOT/pages/cops.adoc @@ -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] diff --git a/docs/modules/ROOT/pages/cops_style.adoc b/docs/modules/ROOT/pages/cops_style.adoc index dc14a2c7bb0..47b8a071f9f 100644 --- a/docs/modules/ROOT/pages/cops_style.adoc +++ b/docs/modules/ROOT/pages/cops_style.adoc @@ -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 |=== diff --git a/lib/rubocop.rb b/lib/rubocop.rb index 96231f293be..6964edbc756 100644 --- a/lib/rubocop.rb +++ b/lib/rubocop.rb @@ -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' diff --git a/lib/rubocop/cop/correctors/alignment_corrector.rb b/lib/rubocop/cop/correctors/alignment_corrector.rb index 32f5d700261..2331343deae 100644 --- a/lib/rubocop/cop/correctors/alignment_corrector.rb +++ b/lib/rubocop/cop/correctors/alignment_corrector.rb @@ -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 diff --git a/lib/rubocop/cop/layout/first_method_argument_line_break.rb b/lib/rubocop/cop/layout/first_method_argument_line_break.rb index d49048e905f..d655c95e64a 100644 --- a/lib/rubocop/cop/layout/first_method_argument_line_break.rb +++ b/lib/rubocop/cop/layout/first_method_argument_line_break.rb @@ -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 diff --git a/lib/rubocop/cop/layout/multiline_method_argument_line_breaks.rb b/lib/rubocop/cop/layout/multiline_method_argument_line_breaks.rb index be266a2354f..390c1adda34 100644 --- a/lib/rubocop/cop/layout/multiline_method_argument_line_breaks.rb +++ b/lib/rubocop/cop/layout/multiline_method_argument_line_breaks.rb @@ -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 diff --git a/lib/rubocop/cop/layout/space_around_operators.rb b/lib/rubocop/cop/layout/space_around_operators.rb index ded974f78d5..e1a7e1922ce 100644 --- a/lib/rubocop/cop/layout/space_around_operators.rb +++ b/lib/rubocop/cop/layout/space_around_operators.rb @@ -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 diff --git a/lib/rubocop/cop/lint/missing_super.rb b/lib/rubocop/cop/lint/missing_super.rb index ccf9d7d1b25..8afc00acfd0 100644 --- a/lib/rubocop/cop/lint/missing_super.rb +++ b/lib/rubocop/cop/lint/missing_super.rb @@ -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 diff --git a/lib/rubocop/cop/mixin/check_line_breakable.rb b/lib/rubocop/cop/mixin/check_line_breakable.rb index 21d8c04c513..826df189c6c 100644 --- a/lib/rubocop/cop/mixin/check_line_breakable.rb +++ b/lib/rubocop/cop/mixin/check_line_breakable.rb @@ -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? @@ -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 diff --git a/lib/rubocop/cop/style/hash_syntax.rb b/lib/rubocop/cop/style/hash_syntax.rb index 66fdfd5a06e..2ba726b8f1b 100644 --- a/lib/rubocop/cop/style/hash_syntax.rb +++ b/lib/rubocop/cop/style/hash_syntax.rb @@ -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. diff --git a/lib/rubocop/cop/style/if_unless_modifier.rb b/lib/rubocop/cop/style/if_unless_modifier.rb index 27bbcc34d43..53d3a36f3b4 100644 --- a/lib/rubocop/cop/style/if_unless_modifier.rb +++ b/lib/rubocop/cop/style/if_unless_modifier.rb @@ -43,8 +43,8 @@ class IfUnlessModifier < Base 'Modifier form of `%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 diff --git a/lib/rubocop/cop/style/redundant_parentheses.rb b/lib/rubocop/cop/style/redundant_parentheses.rb index eaa306f2606..3a040adef50 100644 --- a/lib/rubocop/cop/style/redundant_parentheses.rb +++ b/lib/rubocop/cop/style/redundant_parentheses.rb @@ -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 diff --git a/lib/rubocop/cop/style/sole_nested_conditional.rb b/lib/rubocop/cop/style/sole_nested_conditional.rb new file mode 100644 index 00000000000..17e301dc352 --- /dev/null +++ b/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 `%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 diff --git a/lib/rubocop/cop/style/trailing_underscore_variable.rb b/lib/rubocop/cop/style/trailing_underscore_variable.rb index df3580614de..ef5ec64179e 100644 --- a/lib/rubocop/cop/style/trailing_underscore_variable.rb +++ b/lib/rubocop/cop/style/trailing_underscore_variable.rb @@ -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 diff --git a/lib/rubocop/cop/style/yoda_condition.rb b/lib/rubocop/cop/style/yoda_condition.rb index 3f1fdc03335..932b10a10fa 100644 --- a/lib/rubocop/cop/style/yoda_condition.rb +++ b/lib/rubocop/cop/style/yoda_condition.rb @@ -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 diff --git a/lib/rubocop/rspec/expect_offense.rb b/lib/rubocop/rspec/expect_offense.rb index 1b1569c2922..c64c2359f01 100644 --- a/lib/rubocop/rspec/expect_offense.rb +++ b/lib/rubocop/rspec/expect_offense.rb @@ -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 diff --git a/spec/rubocop/cop/style/sole_nested_conditional_spec.rb b/spec/rubocop/cop/style/sole_nested_conditional_spec.rb new file mode 100644 index 00000000000..44f706dafb3 --- /dev/null +++ b/spec/rubocop/cop/style/sole_nested_conditional_spec.rb @@ -0,0 +1,159 @@ +# frozen_string_literal: true + +RSpec.describe RuboCop::Cop::Style::SoleNestedConditional, :config do + let(:cop_config) do + { 'AllowModifier' => false } + end + + it 'registers an offense when using nested `if` within `if`' do + expect_offense(<<~RUBY) + if foo + if bar + ^^ Consider merging nested conditions into outer `if` conditions. + do_something + end + end + RUBY + end + + it 'registers an offense when using nested `unless` within `if`' do + expect_offense(<<~RUBY) + if foo + unless bar + ^^^^^^ Consider merging nested conditions into outer `if` conditions. + do_something + end + end + RUBY + end + + it 'registers an offense when using nested `if` within `unless`' do + expect_offense(<<~RUBY) + unless foo + if bar + ^^ Consider merging nested conditions into outer `unless` conditions. + do_something + end + end + RUBY + end + + it 'registers an offense when using nested `unless` within `unless`' do + expect_offense(<<~RUBY) + unless foo + unless bar + ^^^^^^ Consider merging nested conditions into outer `unless` conditions. + do_something + end + end + RUBY + end + + it 'does not register an offense when using nested conditional within `elsif`' do + expect_no_offenses(<<~RUBY) + if foo + elsif bar + if baz + end + end + RUBY + end + + it 'registers an offense when using nested modifier conditional' do + expect_offense(<<~RUBY) + if foo + do_something if bar + ^^ Consider merging nested conditions into outer `if` conditions. + end + RUBY + end + + it 'registers an offense for multiple nested conditionals' do + expect_offense(<<~RUBY) + if foo + if bar + ^^ Consider merging nested conditions into outer `if` conditions. + if baz + ^^ Consider merging nested conditions into outer `if` conditions. + do_something + end + end + end + RUBY + end + + it 'registers an offense when using nested conditional and branch contains a comment' do + expect_offense(<<~RUBY) + if foo + # Comment. + if bar + ^^ Consider merging nested conditions into outer `if` conditions. + do_something + end + end + RUBY + end + + it 'does not register an offense when using nested ternary within conditional' do + expect_no_offenses(<<~RUBY) + if foo + bar ? baz : quux + end + RUBY + end + + it 'does not register an offense when no nested conditionals' do + expect_no_offenses(<<~RUBY) + if foo + do_something + end + RUBY + end + + it 'does not register an offense when using nested conditional is not the whole body' do + expect_no_offenses(<<~RUBY) + if foo + if bar + do_something + end + do_something_more + end + RUBY + end + + it 'does not register an offense when nested conditional has an `else` branch' do + expect_no_offenses(<<~RUBY) + if foo + if bar + do_something + else + do_something_else + end + end + RUBY + end + + it 'does not register an offense for nested conditionals when outer conditional has an `else` branch' do + expect_no_offenses(<<~RUBY) + if foo + do_something if bar + else + do_something_else + end + RUBY + end + + context 'when AllowModifier is true' do + let(:cop_config) do + { 'AllowModifier' => true } + end + + it 'does not register an offense when using nested modifier conditional' do + expect_no_offenses(<<~RUBY) + if foo + do_something if bar + end + RUBY + end + end +end