From 887a06d5aae3906f6e608be2ceeae78d34745cff Mon Sep 17 00:00:00 2001 From: Benjamin Quorning Date: Fri, 10 Jul 2020 10:52:19 +0200 Subject: [PATCH 1/2] Don't spend CPU finding the same node twice --- lib/rubocop/cop/rspec/leading_subject.rb | 11 +++-------- 1 file changed, 3 insertions(+), 8 deletions(-) diff --git a/lib/rubocop/cop/rspec/leading_subject.rb b/lib/rubocop/cop/rspec/leading_subject.rb index 9080e9a77..7bc60c64e 100644 --- a/lib/rubocop/cop/rspec/leading_subject.rb +++ b/lib/rubocop/cop/rspec/leading_subject.rb @@ -47,7 +47,7 @@ def check_previous_nodes(node) if offending?(sibling) msg = format(MSG, offending: sibling.method_name) add_offense(node, message: msg) do |corrector| - autocorrect(corrector, node) + autocorrect(corrector, node, sibling) end end @@ -57,21 +57,16 @@ def check_previous_nodes(node) private - def autocorrect(corrector, node) - first_node = find_first_offending_node(node) + def autocorrect(corrector, node, sibling) RuboCop::RSpec::Corrector::MoveNode.new( node, corrector, processed_source - ).move_before(first_node) + ).move_before(sibling) end def offending?(node) let?(node) || hook?(node) || example?(node) end - def find_first_offending_node(node) - node.parent.children.find { |sibling| offending?(sibling) } - end - def in_spec_block?(node) node.each_ancestor(:block).any? do |ancestor| example?(ancestor) From fb10f1e26b2a21c6982d1301052c923812ea6575 Mon Sep 17 00:00:00 2001 From: Benjamin Quorning Date: Tue, 14 Jul 2020 22:30:05 +0200 Subject: [PATCH 2/2] Improve readability of #check_previous_nodes --- lib/rubocop/cop/rspec/leading_subject.rb | 20 +++++++++------ .../rubocop/cop/rspec/leading_subject_spec.rb | 25 +++++++++++++++++++ 2 files changed, 37 insertions(+), 8 deletions(-) diff --git a/lib/rubocop/cop/rspec/leading_subject.rb b/lib/rubocop/cop/rspec/leading_subject.rb index 7bc60c64e..a9645955f 100644 --- a/lib/rubocop/cop/rspec/leading_subject.rb +++ b/lib/rubocop/cop/rspec/leading_subject.rb @@ -43,20 +43,24 @@ def on_block(node) end def check_previous_nodes(node) - node.parent.each_child_node do |sibling| - if offending?(sibling) - msg = format(MSG, offending: sibling.method_name) - add_offense(node, message: msg) do |corrector| - autocorrect(corrector, node, sibling) - end + offending_node(node) do |offender| + msg = format(MSG, offending: offender.method_name) + add_offense(node, message: msg) do |corrector| + autocorrect(corrector, node, offender) end - - break if offending?(sibling) || sibling.equal?(node) end end private + def offending_node(node) + node.parent.each_child_node.find do |sibling| + break if sibling.equal?(node) + + yield sibling if offending?(sibling) + end + end + def autocorrect(corrector, node, sibling) RuboCop::RSpec::Corrector::MoveNode.new( node, corrector, processed_source diff --git a/spec/rubocop/cop/rspec/leading_subject_spec.rb b/spec/rubocop/cop/rspec/leading_subject_spec.rb index d214b3e76..58093f8bf 100644 --- a/spec/rubocop/cop/rspec/leading_subject_spec.rb +++ b/spec/rubocop/cop/rspec/leading_subject_spec.rb @@ -135,4 +135,29 @@ end RUBY end + + it 'checks also when subject is below a non-offending node' do + expect_offense(<<~RUBY) + RSpec.describe do + def helper_method + end + + it { is_expected.to be_present } + + subject { described_class.new } + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Declare `subject` above any other `it` declarations. + end + RUBY + + expect_correction(<<~RUBY) + RSpec.describe do + def helper_method + end + + subject { described_class.new } + it { is_expected.to be_present } + + end + RUBY + end end