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/SoleNestedConditional cop #8390

Merged
merged 1 commit into from Aug 22, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
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][])
* [#8474](https://github.com/rubocop-hq/rubocop/pull/8474): Add new `Lint/DuplicateRequire` cop. ([@fatkodima][])
* [#8472](https://github.com/rubocop-hq/rubocop/issues/8472): Add new `Lint/UselessMethodDefinition` cop. ([@fatkodima][])
Expand Down
8 changes: 8 additions & 0 deletions config/default.yml
Expand Up @@ -4020,6 +4020,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 @@ -485,6 +485,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 @@ -8986,6 +8986,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 @@ -456,6 +456,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