Merge pull request #903 from rubocop-hq/scattered-let-autocorrect

Scattered let autocorrect

CHANGELOG.md

@@ -7,6 +7,7 @@
 * Ignore String constants by `RSpec/Describe`. ([@AlexWayfer][])
 * Drop support for ruby 2.3. ([@bquorning][])
 * Fix multiple cops to detect `let` with proc argument. ([@tejasbubane][])
+* Add autocorrect support for `RSpec/ScatteredLet`. ([@Darhazer][])
 
 ## 1.38.1 (2020-02-15)
 

config/default.yml

@@ -416,6 +416,7 @@ RSpec/ScatteredLet:
   Description: Checks for let scattered across the example group.
   Enabled: true
   StyleGuide: https://www.rubydoc.info/gems/rubocop-rspec/RuboCop/Cop/RSpec/ScatteredLet
+  VersionChanged: '1.39'
 
 RSpec/ScatteredSetup:
   Description: Checks for setup scattered across multiple hooks in an example group.

lib/rubocop-rspec.rb

@@ -22,6 +22,7 @@
 require_relative 'rubocop/rspec/factory_bot'
 require_relative 'rubocop/rspec/final_end_location'
 require_relative 'rubocop/rspec/blank_line_separation'
+require_relative 'rubocop/rspec/corrector/move_node'
 
 RuboCop::RSpec::Inject.defaults!
 

lib/rubocop/cop/rspec/hooks_before_examples.rb

@@ -24,9 +24,6 @@ module RSpec
       #   end
       #
       class HooksBeforeExamples < Cop
-        include RangeHelp
-        include RuboCop::RSpec::FinalEndLocation
-
         MSG = 'Move `%<hook>s` above the examples in the group.'
 
         def_node_matcher :example_or_group?, <<-PATTERN
@@ -45,11 +42,9 @@ def on_block(node)
         def autocorrect(node)
           lambda do |corrector|
             first_example = find_first_example(node.parent)
-            first_example_pos = first_example.loc.expression
-            indent = "\n" + ' ' * first_example.loc.column
-
-            corrector.insert_before(first_example_pos, source(node) + indent)
-            corrector.remove(node_range_with_surrounding_space(node))
+            RuboCop::RSpec::Corrector::MoveNode.new(
+              node, corrector, processed_source
+            ).move_before(first_example)
           end
         end
 
@@ -77,19 +72,6 @@ def check_hooks(node)
         def find_first_example(node)
           node.children.find { |sibling| example_or_group?(sibling) }
         end
-
-        def node_range_with_surrounding_space(node)
-          range = node_range(node)
-          range_by_whole_lines(range, include_final_newline: true)
-        end
-
-        def source(node)
-          node_range(node).source
-        end
-
-        def node_range(node)
-          node.loc.expression.with(end_pos: final_end_location(node).end_pos)
-        end
       end
     end
   end

lib/rubocop/cop/rspec/leading_subject.rb

@@ -32,8 +32,6 @@ module RSpec
       #     it { expect_something_else }
       #
       class LeadingSubject < Cop
-        include RangeHelp
-
         MSG = 'Declare `subject` above any other `%<offending>s` declarations.'
 
         def on_block(node)
@@ -58,10 +56,9 @@ def check_previous_nodes(node)
         def autocorrect(node)
           lambda do |corrector|
             first_node = find_first_offending_node(node)
-            first_node_position = first_node.loc.expression
-            indent = "\n" + ' ' * first_node.loc.column
-            corrector.insert_before(first_node_position, node.source + indent)
-            corrector.remove(node_range(node))
+            RuboCop::RSpec::Corrector::MoveNode.new(
+              node, corrector, processed_source
+            ).move_before(first_node)
           end
         end
 
@@ -75,10 +72,6 @@ def find_first_offending_node(node)
           node.parent.children.find { |sibling| offending?(sibling) }
         end
 
-        def node_range(node)
-          range_by_whole_lines(node.source_range, include_final_newline: true)
-        end
-
         def in_spec_block?(node)
           node.each_ancestor(:block).any? do |ancestor|
             example?(ancestor)

lib/rubocop/cop/rspec/let_before_examples.rb

@@ -31,9 +31,6 @@ module RSpec
       #     expect(some).to be
       #   end
       class LetBeforeExamples < Cop
-        include RangeHelp
-        include RuboCop::RSpec::FinalEndLocation
-
         MSG = 'Move `let` before the examples in the group.'
 
         def_node_matcher :example_or_group?, <<-PATTERN
@@ -52,11 +49,9 @@ def on_block(node)
         def autocorrect(node)
           lambda do |corrector|
             first_example = find_first_example(node.parent)
-            first_example_pos = first_example.loc.expression
-            indent = "\n" + ' ' * first_example.loc.column
-
-            corrector.insert_before(first_example_pos, source(node) + indent)
-            corrector.remove(node_range_with_surrounding_space(node))
+            RuboCop::RSpec::Corrector::MoveNode.new(
+              node, corrector, processed_source
+            ).move_before(first_example)
           end
         end
 
@@ -80,19 +75,6 @@ def check_let_declarations(node)
         def find_first_example(node)
           node.children.find { |sibling| example_or_group?(sibling) }
         end
-
-        def node_range_with_surrounding_space(node)
-          range = node_range(node)
-          range_by_whole_lines(range, include_final_newline: true)
-        end
-
-        def source(node)
-          node_range(node).source
-        end
-
-        def node_range(node)
-          node.loc.expression.with(end_pos: final_end_location(node).end_pos)
-        end
       end
     end
   end

lib/rubocop/cop/rspec/scattered_let.rb

@@ -35,6 +35,15 @@ def on_block(node)
           check_let_declarations(node.body)
         end
 
+        def autocorrect(node)
+          lambda do |corrector|
+            first_let = find_first_let(node.parent)
+            RuboCop::RSpec::Corrector::MoveNode.new(
+              node, corrector, processed_source
+            ).move_after(first_let)
+          end
+        end
+
         private
 
         def check_let_declarations(body)
@@ -47,6 +56,10 @@ def check_let_declarations(body)
             add_offense(node)
           end
         end
+
+        def find_first_let(node)
+          node.children.find { |child| let?(child) }
+        end
       end
     end
   end

lib/rubocop/rspec/corrector/move_node.rb

@@ -0,0 +1,52 @@
+# frozen_string_literal: true
+
+module RuboCop
+  module RSpec
+    module Corrector
+      # Helper methods to move a node
+      class MoveNode
+        include RuboCop::Cop::RangeHelp
+        include RuboCop::RSpec::FinalEndLocation
+
+        attr_reader :original, :corrector, :processed_source
+
+        def initialize(node, corrector, processed_source)
+          @original = node
+          @corrector = corrector
+          @processed_source = processed_source # used by RangeHelp
+        end
+
+        def move_before(other) # rubocop:disable Metrics/AbcSize
+          position = other.loc.expression
+          indent = "\n" + ' ' * other.loc.column
+
+          corrector.insert_before(position, source(original) + indent)
+          corrector.remove(node_range_with_surrounding_space(original))
+        end
+
+        def move_after(other)
+          position = final_end_location(other)
+          indent = "\n" + ' ' * other.loc.column
+
+          corrector.insert_after(position, indent + source(original))
+          corrector.remove(node_range_with_surrounding_space(original))
+        end
+
+        private
+
+        def source(node)
+          node_range(node).source
+        end
+
+        def node_range(node)
+          node.loc.expression.with(end_pos: final_end_location(node).end_pos)
+        end
+
+        def node_range_with_surrounding_space(node)
+          range = node_range(node)
+          range_by_whole_lines(range, include_final_newline: true)
+        end
+      end
+    end
+  end
+end

manual/cops_rspec.md

@@ -2723,7 +2723,7 @@ EnforcedStyle | `and_return` | `and_return`, `block`
 
 Enabled by default | Supports autocorrection
 --- | ---
-Enabled | No
+Enabled | Yes
 
 Checks for let scattered across the example group.
 
@@ -2751,6 +2751,12 @@ describe Foo do
 end
 ```
 
+### Configurable attributes
+
+Name | Default value | Configurable values
+--- | --- | ---
+VersionChanged | `1.39` | String
+
 ### References
 
 * [https://www.rubydoc.info/gems/rubocop-rspec/RuboCop/Cop/RSpec/ScatteredLet](https://www.rubydoc.info/gems/rubocop-rspec/RuboCop/Cop/RSpec/ScatteredLet)

spec/rubocop/cop/rspec/scattered_let_spec.rb

@@ -7,16 +7,86 @@
     expect_offense(<<-RUBY)
       RSpec.describe User do
         let(:a) { a }
-        subject { User }
+        it { expect(subject.foo).to eq(a) }
         let(:b) { b }
         ^^^^^^^^^^^^^ Group all let/let! blocks in the example group together.
       end
     RUBY
+
+    expect_correction(<<-RUBY)
+      RSpec.describe User do
+        let(:a) { a }
+        let(:b) { b }
+        it { expect(subject.foo).to eq(a) }
+      end
+    RUBY
+  end
+
+  it 'works with heredocs' do
+    expect_offense(<<-RUBY)
+      describe User do
+        let(:a) { <<-BAR }
+          hello
+          world
+        BAR
+        it { expect(subject.foo).to eq(a) }
+        let(:b) { <<-BAZ }
+        ^^^^^^^^^^^^^^^^^^ Group all let/let! blocks in the example group together.
+          again
+        BAZ
+      end
+    RUBY
+
+    expect_correction(<<-RUBY)
+      describe User do
+        let(:a) { <<-BAR }
+          hello
+          world
+        BAR
+        let(:b) { <<-BAZ }
+          again
+        BAZ
+        it { expect(subject.foo).to eq(a) }
+      end
+    RUBY
+  end
+
+  it 'flags `let` at different nesting levels' do
+    expect_offense(<<-RUBY)
+      describe User do
+        let(:a) { a }
+        it { expect(subject.foo).to eq(a) }
+
+        describe '#property' do
+          let(:c) { c }
+
+          it { expect(subject.property).to eq c }
+
+          let(:d) { d }
+          ^^^^^^^^^^^^^ Group all let/let! blocks in the example group together.
+        end
+      end
+    RUBY
+
+    expect_correction(<<-RUBY)
+      describe User do
+        let(:a) { a }
+        it { expect(subject.foo).to eq(a) }
+
+        describe '#property' do
+          let(:c) { c }
+          let(:d) { d }
+
+          it { expect(subject.property).to eq c }
+
+        end
+      end
+    RUBY
   end
 
   it 'doesnt flag `let!` in the middle of multiple `let`s' do
     expect_no_offenses(<<-RUBY)
-      RSpec.describe User do
+      describe User do
         subject { User }
 
         let(:a) { a }
@@ -26,14 +96,41 @@
     RUBY
   end
 
+  it 'flags scattered `let!`s' do
+    expect_offense(<<-RUBY)
+      describe User do
+        let!(:a) { a }
+        it { expect(subject.foo).to eq(a) }
+        let!(:c) { c }
+        ^^^^^^^^^^^^^^ Group all let/let! blocks in the example group together.
+      end
+    RUBY
+
+    expect_correction(<<-RUBY)
+      describe User do
+        let!(:a) { a }
+        let!(:c) { c }
+        it { expect(subject.foo).to eq(a) }
+      end
+    RUBY
+  end
+
   it 'flags `let` with proc argument' do
     expect_offense(<<-RUBY)
-      RSpec.describe User do
+      describe User do
         let(:a) { a }
-        subject { User }
+        it { expect(subject.foo).to eq(a) }
         let(:user, &args[:build_user])
         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Group all let/let! blocks in the example group together.
       end
     RUBY
+
+    expect_correction(<<-RUBY)
+      describe User do
+        let(:a) { a }
+        let(:user, &args[:build_user])
+        it { expect(subject.foo).to eq(a) }
+      end
+    RUBY
   end
 end