Commit f008b6a9 authored by Jacopo's avatar Jacopo

Updates after 2nd review

parent 81a22b76
...@@ -111,7 +111,7 @@ namespace :gitlab do ...@@ -111,7 +111,7 @@ namespace :gitlab do
puts " - #{project.full_path} (id: #{project.id})".color(:red) puts " - #{project.full_path} (id: #{project.id})".color(:red)
return if counter >= limit # rubocop:disable Lint/NonLocalExitFromIterator return if counter >= limit # rubocop:disable Lint/NonLocalExitFromIterator, Cop/AvoidReturnFromBlocks
end end
end end
end end
...@@ -132,7 +132,7 @@ namespace :gitlab do ...@@ -132,7 +132,7 @@ namespace :gitlab do
puts " - #{upload.path} (id: #{upload.id})".color(:red) puts " - #{upload.path} (id: #{upload.id})".color(:red)
return if counter >= limit # rubocop:disable Lint/NonLocalExitFromIterator return if counter >= limit # rubocop:disable Lint/NonLocalExitFromIterator, Cop/AvoidReturnFromBlocks
end end
end end
end end
......
...@@ -3,6 +3,7 @@ ...@@ -3,6 +3,7 @@
module RuboCop module RuboCop
module Cop module Cop
# Checks for break inside strong_memoize blocks. # Checks for break inside strong_memoize blocks.
# For more information see: https://gitlab.com/gitlab-org/gitlab-ce/issues/42889
# #
# @example # @example
# # bad # # bad
...@@ -26,11 +27,10 @@ module RuboCop ...@@ -26,11 +27,10 @@ module RuboCop
block_body = node.body block_body = node.body
return unless block_body return unless block_body
return unless node.method_name == :strong_memoize
block_body.each_node(:break) do |break_node| block_body.each_node(:break) do |break_node|
container_block = container_block_for(break_node) next if container_block_for(break_node) != node
next if container_block.method_name != :strong_memoize
next if container_block != node
add_offense(break_node) add_offense(break_node)
end end
...@@ -39,7 +39,7 @@ module RuboCop ...@@ -39,7 +39,7 @@ module RuboCop
private private
def container_block_for(current_node) def container_block_for(current_node)
current_node = current_node.parent until current_node.type == :block current_node = current_node.parent until current_node.type == :block && current_node.method_name == :strong_memoize
current_node current_node
end end
......
...@@ -3,7 +3,7 @@ ...@@ -3,7 +3,7 @@
module RuboCop module RuboCop
module Cop module Cop
# Checks for return inside blocks. # Checks for return inside blocks.
# Whitelisted methods are ignored: each, each_filename, times, loop, define_method. # For more information see: https://gitlab.com/gitlab-org/gitlab-ce/issues/42889
# #
# @example # @example
# # bad # # bad
...@@ -22,40 +22,46 @@ module RuboCop ...@@ -22,40 +22,46 @@ module RuboCop
# #
class AvoidReturnFromBlocks < RuboCop::Cop::Cop class AvoidReturnFromBlocks < RuboCop::Cop::Cop
MSG = 'Do not return from a block, use next or break instead.' MSG = 'Do not return from a block, use next or break instead.'
WHITELISTED_METHODS = %i[each each_filename times loop define_method lambda].freeze WHITELISTED_METHODS = %i[each each_filename times loop define_method lambda helpers class_methods describe included namespace validations].freeze
def on_block(node) def on_block(node)
block_body = node.body block_body = node.body
return unless block_body return unless block_body
return if WHITELISTED_METHODS.include?(node.method_name) return unless top_block?(node)
block_body.each_node(:return) do |return_node| block_body.each_node(:return) do |return_node|
next if container_block_for(return_node) != node next if contained_blocks(node, return_node).all?(&method(:whitelisted?))
next if container_block_whitelisted?(return_node)
next if return_inside_method_definition?(return_node)
add_offense(return_node) add_offense(return_node)
end end
end end
private def top_block?(node)
current_node = node
top_block = nil
def container_block_for(current_node) while current_node && current_node.type != :def
current_node = current_node.parent until current_node.type == :block top_block = current_node if current_node.type == :block
current_node = current_node.parent
end
current_node top_block == node
end end
def container_block_whitelisted?(current_node) def contained_blocks(node, current_node)
WHITELISTED_METHODS.include?(container_block_for(current_node).method_name) blocks = []
end
def return_inside_method_definition?(current_node) until node == current_node
current_node = current_node.parent until %i[def block].include?(current_node.type) blocks << current_node if current_node.type == :block
current_node = current_node.parent
end
blocks << node
end
# if we found :def before :block in the nodes hierarchy def whitelisted?(block_node)
current_node.type == :def WHITELISTED_METHODS.include?(block_node.method_name)
end end
end end
end end
......
...@@ -26,6 +26,19 @@ describe RuboCop::Cop::AvoidBreakFromStrongMemoize do ...@@ -26,6 +26,19 @@ describe RuboCop::Cop::AvoidBreakFromStrongMemoize do
expect(offense.message).to eq('Do not use break inside strong_memoize, use next instead.') expect(offense.message).to eq('Do not use break inside strong_memoize, use next instead.')
end end
it 'flags violation for break inside strong_memoize nested blocks' do
source = <<~RUBY
strong_memoize do
items.each do |item|
break item
end
end
RUBY
inspect_source(source)
expect(cop.offenses.size).to eq(1)
end
it "doesn't flag violation for next inside strong_memoize" do it "doesn't flag violation for next inside strong_memoize" do
source = <<~RUBY source = <<~RUBY
strong_memoize(:result) do strong_memoize(:result) do
......
...@@ -53,8 +53,8 @@ describe RuboCop::Cop::AvoidReturnFromBlocks do ...@@ -53,8 +53,8 @@ describe RuboCop::Cop::AvoidReturnFromBlocks do
end end
end end
%w[each each_filename times loop define_method].each do |example| %i[each each_filename times loop define_method lambda helpers class_methods describe included namespace validations].each do |whitelisted_method|
it_behaves_like 'examples with whitelisted method', example it_behaves_like 'examples with whitelisted method', whitelisted_method
end end
it "doesn't flag violation for return inside a lambda" do it "doesn't flag violation for return inside a lambda" do
...@@ -83,22 +83,6 @@ describe RuboCop::Cop::AvoidReturnFromBlocks do ...@@ -83,22 +83,6 @@ describe RuboCop::Cop::AvoidReturnFromBlocks do
expect(cop.offenses).to be_empty expect(cop.offenses).to be_empty
end end
it "doesn't flag violation for whitelisted method inside nested blocks" do
source = <<~RUBY
call do
call_2 do
items.each do |item|
do_something
return if something_else
end
end
end
RUBY
inspect_source(source)
expect(cop.offenses).to be_empty
end
it "doesn't flag violation for next inside a block" do it "doesn't flag violation for next inside a block" do
source = <<~RUBY source = <<~RUBY
call do call do
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment