Refactor/manual rubocop update (#268)

* Remove most meta-programming from event and introduce more properties

* Don't expose events as a public API

* Reduce ABC score of Parser

* Improve ContextWording in specs

* Update changelog entry for rubocop refactors

* Add pending blocker for spec

CHANGELOG.md

@@ -11,8 +11,8 @@ Please visit [cucumber/CONTRIBUTING.md](https://github.com/cucumber/cucumber/blo
 ## [Unreleased]
 ### Changed
 - Now using a 2-tiered changelog to avoid any bugs when using polyglot-release
-- More refactoring of the repo by fixing up a bunch of manual rubocop offenses (See PR for details)
-  ([#259](https://github.com/cucumber/cucumber-ruby-core/pull/259) [#262](https://github.com/cucumber/cucumber-ruby-core/pull/262))
+- More refactoring of the repo by fixing up a bunch of manual rubocop offenses (See PR's for details)
+  ([#259](https://github.com/cucumber/cucumber-ruby-core/pull/259) [#262](https://github.com/cucumber/cucumber-ruby-core/pull/262) [#268](https://github.com/cucumber/cucumber-ruby-core/pull/268))
 - In all `Summary` and `Result` classes, changed the `strict` argument into a keyword argument.
   See upgrading notes for [13.0.0.md](upgrading_notes/13.0.0.md#upgrading-to-1300)
   ([#261](https://github.com/cucumber/cucumber-ruby-core/pull/261))

lib/cucumber/core/event.rb

@@ -5,33 +5,36 @@ module Core
     class Event
       # Macro to generate new sub-classes of {Event} with
       # attribute readers.
-      def self.new(*attributes)
+      def self.new(*events)
         # Use normal constructor for subclasses of Event
         return super if ancestors.index(Event) > 0
 
         Class.new(Event) do
-          attr_reader(*attributes)
+          attr_reader(*events)
 
-          define_method(:initialize) do |*args|
-            attributes.zip(args) do |name, value|
+          define_method(:initialize) do |*attributes|
+            events.zip(attributes) do |name, value|
               instance_variable_set("@#{name}".to_sym, value)
             end
           end
 
-          define_method(:attributes) do
-            attributes.map { |attribute| send(attribute) }
+          def attributes
+            instance_variables.map { |var| instance_variable_get(var) }
           end
 
-          define_method(:to_h) do
-            attributes.reduce({}) { |result, attribute|
-              result[attribute] = send(attribute)
-              result
-            }
+          def to_h
+            events.zip(attributes).to_h
           end
 
-          define_method(:event_id) do
+          def event_id
             self.class.event_id
           end
+
+          private
+
+          def events
+            instance_variables.map { |var| (var[1..-1]).to_sym }
+          end
         end
       end
 
@@ -44,11 +47,13 @@ def event_id
         private
 
         def underscore(string)
-          string.to_s.gsub('::', '/').
-          gsub(/([A-Z]+)([A-Z][a-z])/, '\1_\2').
-          gsub(/([a-z\d])([A-Z])/, '\1_\2').
-          tr('-', '_').
-          downcase
+          string
+            .to_s
+            .gsub('::', '/').
+            gsub(/([A-Z]+)([A-Z][a-z])/, '\1_\2').
+            gsub(/([a-z\d])([A-Z])/, '\1_\2').
+            tr('-', '_').
+            downcase
         end
       end
     end

lib/cucumber/core/gherkin/parser.rb

@@ -44,17 +44,34 @@ def source_messages(document)
         end
 
         def process(message, document)
+          generate_envelope(message)
+          update_gherkin_query(message)
+
+          case type?(message)
+          when :gherkin_document; then event_bus.gherkin_source_parsed(message.gherkin_document)
+          when :pickle;           then receiver.pickle(message.pickle)
+          when :parse_error;      then raise ParseError.new("#{document.uri}: #{message.parse_error.message}")
+          else                    raise "Unknown message: #{message.to_hash}"
+          end
+        end
+
+        def generate_envelope(message)
           event_bus.envelope(message)
+        end
+
+        def update_gherkin_query(message)
           gherkin_query.update(message)
+        end
 
+        def type?(message)
           if !message.gherkin_document.nil?
-            event_bus.gherkin_source_parsed(message.gherkin_document)
+            :gherkin_document
           elsif !message.pickle.nil?
-            receiver.pickle(message.pickle)
+            :pickle
           elsif message.parse_error
-            raise ParseError.new("#{document.uri}: #{message.parse_error.message}")
+            :parse_error
           else
-            raise "Unknown message: #{message.to_hash}"
+            :unknown
           end
         end
       end

spec/cucumber/core/compiler_spec.rb

@@ -126,7 +126,7 @@ def self.stubs(*names)
     end
   end
 
-  context 'compiling scenario outlines' do
+  context 'when compiling scenario outlines' do
     it 'compiles a scenario outline to test cases' do
       gherkin_documents = [
         gherkin do
@@ -191,8 +191,8 @@ def self.stubs(*names)
     end
   end
 
-  context 'empty scenarios' do
-    it 'does create test cases for them' do
+  context 'with empty scenarios' do
+    it 'creates test cases' do
       compile([empty_gherkin_document]) do |visitor|
         expect(visitor).to receive(:test_case).once.ordered
         expect(visitor).to receive(:done).once.ordered

spec/cucumber/core/event_bus_spec.rb

@@ -17,7 +17,7 @@ class TestEvent < Core::Event.new(:some_attribute)
       let(:event_bus) { described_class.new(registry) }
       let(:registry) { { test_event: Events::TestEvent, another_test_event: Events::AnotherTestEvent } }
 
-      context 'broadcasting events' do
+      context 'when broadcasting events' do
         it 'can broadcast by calling a method named after the event ID' do
           called = false
           event_bus.on(:test_event) { called = true }
@@ -79,7 +79,7 @@ class TestEvent < Core::Event.new(:some_attribute)
         end
       end
 
-      context 'subscribing to events' do
+      context 'when subscribing to events' do
         let(:regular_handler) do
           Class.new do
             attr_reader :received_payload

spec/cucumber/core/event_spec.rb

@@ -13,7 +13,7 @@
     it 'generates events with attributes' do
       my_event_type = described_class.new(:foo, :bar)
       my_event = my_event_type.new(1, 2)
-      expect(my_event.attributes).to eq [1, 2]
+      expect(my_event.attributes).to eq([1, 2])
       expect(my_event.foo).to eq(1)
       expect(my_event.bar).to eq(2)
     end

spec/cucumber/core/filter_spec.rb

@@ -35,7 +35,7 @@
       compile [doc], receiver, [my_filter]
     end
 
-    context 'customizing by subclassing' do
+    context 'when customizing using a subclass' do
       let(:basic_blanking_filter) do
         # Each filter implicitly gets a :receiver attribute
         # that you need to call with the new test case
@@ -78,7 +78,7 @@ def test_case(test_case)
       end
     end
 
-    context 'customizing by using a block' do
+    context 'when customizing using a block' do
       let(:block_blanking_filter) do
         Class.new(described_class.new) do
           def test_case(test_case)

spec/cucumber/core/gherkin/writer_spec.rb

@@ -6,19 +6,17 @@
 describe Cucumber::Core::Gherkin::Writer do
   include described_class
 
-  context 'specifying uri' do
-    it 'generates a uri by default' do
-      source = gherkin { feature }
-      expect(source.uri).to eq 'features/test.feature'
-    end
+  it 'generates a uri by default' do
+    source = gherkin { feature }
+    expect(source.uri).to eq 'features/test.feature'
+  end
 
-    it 'allows you to specify a URI' do
-      source = gherkin('features/path/to/my.feature') { feature }
-      expect(source.uri).to eq 'features/path/to/my.feature'
-    end
+  it 'allows you to specify a URI' do
+    source = gherkin('features/path/to/my.feature') { feature }
+    expect(source.uri).to eq 'features/path/to/my.feature'
   end
 
-  context 'a feature' do
+  context 'for a feature' do
     it 'generates the feature statement' do
       source = gherkin { feature }
       expect(source).to eq "Feature:\n"
@@ -208,7 +206,7 @@
         FEATURE
       end
 
-      context 'and examples table' do
+      context 'with an examples table' do
         it 'can have a description' do
           source = gherkin do
             feature do

spec/cucumber/core/report/summary_spec.rb

@@ -23,7 +23,7 @@ module Report
 
         before(:each) { @summary = described_class.new(event_bus) }
 
-        context 'test case summary' do
+        describe 'test case summary' do
           let(:test_case) { double }
 
           it 'counts passed test cases' do
@@ -83,7 +83,7 @@ module Report
           end
         end
 
-        context 'test step summary' do
+        describe 'test step summary' do
           context 'with test steps from gherkin steps' do
             let(:test_step) { instance_double(Cucumber::Core::Test::Step, hook?: false) }
 

spec/cucumber/core/test/action_spec.rb

@@ -7,13 +7,11 @@ module Cucumber
   module Core
     module Test
       describe Action do
-        context 'constructed without a block' do
-          it 'raises an error' do
-            expect { described_class.new }.to raise_error(ArgumentError)
-          end
+        it 'raises an error if created without a block' do
+          expect { described_class.new }.to raise_error(ArgumentError)
         end
 
-        context 'location' do
+        describe '#location' do
           context 'with location passed to the constructor' do
             let(:location) { double }
 
@@ -33,7 +31,7 @@ module Test
           end
         end
 
-        context 'executing' do
+        context 'when executing' do
           it 'executes the block passed to the constructor' do
             executed = false
             action = described_class.new { executed = true }
@@ -105,7 +103,7 @@ module Test
           end
         end
 
-        context 'skipping' do
+        context 'when skipped' do
           it 'does not execute the block' do
             executed = false
             action = described_class.new { executed = true }
@@ -125,19 +123,19 @@ module Test
         let(:action) { described_class.new(location) }
         let(:test_step) { double }
 
-        context 'location' do
+        describe '#location' do
           it 'returns the location passed to the constructor' do
             expect(action.location).to be location
           end
         end
 
-        context 'executing' do
+        describe '#execute' do
           it 'returns an undefined result' do
             expect(action.execute).to be_undefined
           end
         end
 
-        context 'skipping' do
+        describe '#skip' do
           it 'returns an undefined result' do
             expect(action.skip).to be_undefined
           end

spec/cucumber/core/test/case_spec.rb

@@ -18,7 +18,7 @@
   let(:test_case) { described_class.new(id, name, test_steps, location, tags, language) }
   let(:test_steps) { [double, double] }
 
-  context 'describing itself' do
+  context 'when describing itself' do
     let(:visitor) { double }
     let(:args) { double }
 
@@ -64,8 +64,8 @@
     end
   end
 
-  describe 'matching tags' do
-    let(:tags) { ['@a', '@b', '@c'].map { |value| Cucumber::Core::Test::Tag.new(location, value) } }
+  context 'when matching tags' do
+    let(:tags) { %w[@a @b @c].map { |value| Cucumber::Core::Test::Tag.new(location, value) } }
 
     it 'matches tags using tag expressions' do
       expect(test_case).to be_match_tags(['@a and @b'])
@@ -80,7 +80,7 @@
     end
   end
 
-  describe 'matching names' do
+  context 'when matching names' do
     let(:name) { 'scenario' }
 
     it 'matches names against regexp' do

spec/cucumber/core/test/result_spec.rb

@@ -26,10 +26,8 @@ module Test
             expect(result.to_s).to eq '✓'
           end
 
-          it 'converts to a Cucumber::Message::TestResult' do
-            message = result.to_message
-
-            expect(message.status).to eq(Cucumber::Messages::TestStepResultStatus::PASSED)
+          it 'converts to a `Cucumber::Message::TestResult`' do
+            expect(result.to_message.status).to eq(Cucumber::Messages::TestStepResultStatus::PASSED)
           end
 
           it 'has a duration' do
@@ -336,7 +334,7 @@ module Test
           end
 
           describe '#strict?' do
-            context 'no type argument' do
+            context 'without a type argument' do
               it 'returns true if any result type is set to strict' do
                 strict_configuration.set_strict(false, :pending)
                 expect(strict_configuration).not_to be_strict
@@ -346,7 +344,7 @@ module Test
               end
             end
 
-            context 'with type argument' do
+            context 'with a type argument' do
               it 'returns true if the specified result type is set to strict' do
                 strict_configuration.set_strict(false, :pending)
                 strict_configuration.set_strict(true, :flaky)
@@ -415,7 +413,7 @@ module Test
             expect(summary.total).to eq(1)
           end
 
-          it 'counts abitrary raisable results' do
+          it 'counts arbitrary raiseable results' do
             flickering = Class.new(Result::Raisable) do
               def describe_to(visitor, *args)
                 visitor.flickering(*args)
@@ -461,7 +459,7 @@ def describe_to(visitor, *args)
             expect(summary.exceptions).to eq [exception]
           end
 
-          context 'ok? result' do
+          describe '#ok?' do
             it 'passed result is ok' do
               passed.describe_to(summary)
 
@@ -507,7 +505,7 @@ def describe_to(visitor, *args)
           subject(:duration) { described_class.new(10) }
 
           it '#nanoseconds can be accessed in #tap' do
-            expect(duration.tap { |duration| @duration = duration.nanoseconds }).to eq duration
+            expect(duration.tap { |duration| @duration = duration.nanoseconds }).to eq(duration)
             expect(@duration).to eq(10)
           end
         end