Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions lib/rubocop-rspec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
require_relative 'rubocop/cop/rspec/mixin/location_help'
require_relative 'rubocop/cop/rspec/mixin/metadata'
require_relative 'rubocop/cop/rspec/mixin/namespace'
require_relative 'rubocop/cop/rspec/mixin/repeated_items'
require_relative 'rubocop/cop/rspec/mixin/skip_or_pending'
require_relative 'rubocop/cop/rspec/mixin/top_level_group'
require_relative 'rubocop/cop/rspec/mixin/variable'
Expand Down
36 changes: 36 additions & 0 deletions lib/rubocop/cop/rspec/mixin/repeated_items.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
# frozen_string_literal: true

module RuboCop
module Cop
module RSpec
# Helps find repeated items in a collection
#
# Provides a generic method to find repeated items by grouping them
# by a key and returning pairs of [item, repeated_lines] for items
# that appear more than once.
module RepeatedItems
# Groups items by key and returns only groups with more than one item
#
# @param items [Enumerable] the filtered collection to group
# @param key_proc [Proc] block returning the grouping key for each item
# @return [Array<Array>] array of groups containing more than one item
# that share the same key and there are multiple items in the group
def find_repeated_groups(items, key_proc:)
items
.group_by(&key_proc)
.values
.reject(&:one?)
end
Comment on lines +18 to +23
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't it be simpler to just use a block instead of passing a Proc instance as an argument?

Suggested change
def find_repeated_groups(items, key_proc:)
items
.group_by(&key_proc)
.values
.reject(&:one?)
end
def find_repeated_groups(items, &block)
items
.group_by(&block)
.values
.reject(&:one?)
end

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I actually found the code a bit confusing, when I tried rewriting it with an implicit block just now. If we do do that however, could we rename the block argument from &block to e.g. &signature_proc?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That makes sense.

Without keyword arguments to label things, it can be hard to tell what's going on from the caller’s side. In that case, the original way of passing a Proc via keyword arguments definitely has its perks. Alternatively, it might improve things somewhat to change the method name to something like find_repeated_groups_by, which clearly implies that grouping logic is expected.

Since both approaches seem to have their pros and cons, I felt that leaving it as it is would also be fine 👌


# Maps a group of items to pairs of [item, repeated_lines]
#
# @param items [Array] array of items that share the same key
# @return [Array<Array>] array of [item, repeated_lines] pairs
def add_repeated_lines(items)
repeated_lines = items.map(&:first_line)
items.map { |item| [item, repeated_lines - [item.first_line]] }
end
end
end
end
end
10 changes: 6 additions & 4 deletions lib/rubocop/cop/rspec/repeated_example.rb
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,8 @@ module RSpec
# end
#
class RepeatedExample < Base
include RepeatedItems

MSG = "Don't repeat examples within an example group. " \
'Repeated on line(s) %<lines>s.'

Expand All @@ -32,10 +34,10 @@ def on_block(node) # rubocop:disable InternalAffairs/NumblockHandler
def find_repeated_examples(node)
examples = RuboCop::RSpec::ExampleGroup.new(node).examples

examples
.group_by { |example| build_example_signature(example) }
.values
.select { |group| group.size > 1 }
find_repeated_groups(
examples,
key_proc: ->(example) { build_example_signature(example) }
)
end

def build_example_signature(example)
Expand Down
16 changes: 6 additions & 10 deletions lib/rubocop/cop/rspec/repeated_example_group_body.rb
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@ module RSpec
#
class RepeatedExampleGroupBody < Base
include SkipOrPending
include RepeatedItems

MSG = 'Repeated %<group>s block body on line(s) %<loc>s'

Expand Down Expand Up @@ -72,19 +73,14 @@ def on_begin(node)
private

def repeated_group_bodies(node)
node
.children
items = node.children
.select { |child| example_group_with_body?(child) }
.reject { |child| skip_or_pending_inside_block?(child) }
.group_by { |group| signature_keys(group) }
.values
.reject(&:one?)
.flat_map { |groups| add_repeated_lines(groups) }
end

def add_repeated_lines(groups)
repeated_lines = groups.map(&:first_line)
groups.map { |group| [group, repeated_lines - [group.first_line]] }
find_repeated_groups(
items,
key_proc: ->(group) { signature_keys(group) }
).flat_map { |group| add_repeated_lines(group) }
end

def signature_keys(group)
Expand Down
16 changes: 6 additions & 10 deletions lib/rubocop/cop/rspec/repeated_example_group_description.rb
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@ module RSpec
#
class RepeatedExampleGroupDescription < Base
include SkipOrPending
include RepeatedItems

MSG = 'Repeated %<group>s block description on line(s) %<loc>s'

Expand Down Expand Up @@ -71,20 +72,15 @@ def on_begin(node)
private

def repeated_group_descriptions(node)
node
.children
items = node.children
.select { |child| example_group?(child) }
.reject { |child| skip_or_pending_inside_block?(child) }
.reject { |child| empty_description?(child) }
.group_by { |group| doc_string_and_metadata(group) }
.values
.reject(&:one?)
.flat_map { |groups| add_repeated_lines(groups) }
end

def add_repeated_lines(groups)
repeated_lines = groups.map(&:first_line)
groups.map { |group| [group, repeated_lines - [group.first_line]] }
find_repeated_groups(
items,
key_proc: ->(group) { doc_string_and_metadata(group) }
).flat_map { |group| add_repeated_lines(group) }
end

def message(group, repeats)
Expand Down
19 changes: 8 additions & 11 deletions lib/rubocop/cop/rspec/repeated_include_example.rb
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,8 @@ module RSpec
# end
#
class RepeatedIncludeExample < Base
include RepeatedItems

MSG = 'Repeated include of shared_examples %<name>s ' \
'on line(s) %<repeat>s'

Expand Down Expand Up @@ -73,25 +75,20 @@ def on_begin(node)
private

def repeated_include_examples(node)
node
.children
items = node.children
.select { |child| literal_include_examples?(child) }
.group_by { |child| signature_keys(child) }
.values
.reject(&:one?)
.flat_map { |items| add_repeated_lines(items) }

find_repeated_groups(
items,
key_proc: ->(child) { signature_keys(child) }
).flat_map { |group| add_repeated_lines(group) }
end

def literal_include_examples?(node)
include_examples?(node) &&
node.arguments.all?(&:recursive_literal_or_const?)
end

def add_repeated_lines(items)
repeated_lines = items.map(&:first_line)
items.map { |item| [item, repeated_lines - [item.first_line]] }
end

def signature_keys(item)
item.arguments
end
Expand Down
14 changes: 6 additions & 8 deletions lib/rubocop/cop/rspec/scattered_setup.rb
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@ module RSpec
class ScatteredSetup < Base
include FinalEndLocation
include RangeHelp
include RepeatedItems
extend AutoCorrector

MSG = 'Do not define multiple `%<hook_name>s` hooks in the same ' \
Expand All @@ -63,17 +64,14 @@ def on_block(node) # rubocop:disable InternalAffairs/NumblockHandler
private

def repeated_hooks(node) # rubocop:disable Metrics/CyclomaticComplexity
hooks = RuboCop::RSpec::ExampleGroup.new(node)
.hooks
hooks = RuboCop::RSpec::ExampleGroup.new(node).hooks
.reject(&:inside_class_method?)
.select { |hook| hook.knowable_scope? && hook.name != :around }
.group_by { |hook| [hook.name, hook.scope, hook.metadata] }
.values
.reject(&:one?)

hooks.map do |hook|
hook.map(&:to_node)
end
find_repeated_groups(
hooks,
key_proc: ->(hook) { [hook.name, hook.scope, hook.metadata] }
).map { |hook_group| hook_group.map(&:to_node) }
end

def lines_msg(numbers)
Expand Down