Merge pull request #912 from flippercloud/delegation

Fix all the warnings and turn them on for CI

Author: jnunemaker

Gemfile

@@ -31,6 +31,7 @@ gem 'mysql2'
 gem 'pg'
 gem 'cuprite'
 gem 'puma'
+gem 'warning'
 
 group(:guard) do
   gem 'guard'

Guardfile

@@ -10,7 +10,7 @@ rspec_options = {
   all_after_pass: false,
   all_on_start: false,
   failed_mode: :keep,
-  cmd: 'bundle exec rspec',
+  cmd: 'bundle exec ruby -w $(which rspec)',
 }
 
 guard 'rspec', rspec_options do

lib/flipper/adapters/http/error.rb

@@ -20,7 +20,7 @@ def initialize(response)
             if more_info = data["more_info"]
               message << "\n#{data["more_info"]}"
             end
-          rescue => exception
+          rescue
             # welp we tried
           end
 

lib/flipper/adapters/poll.rb

@@ -25,7 +25,7 @@ def initialize(poller, adapter)
         if adapter.features.empty?
           begin
             @poller.sync
-          rescue => error
+          rescue
             # TODO: Warn here that it's possible that no data has been synced
             # and flags are being evaluated without flag data being present
             # until a sync completes. We rescue to avoid flipper being down

lib/flipper/api/v1/actions/expression_gate.rb

@@ -18,7 +18,7 @@ def post
               feature.enable_expression expression
               decorated_feature = Decorators::Feature.new(feature)
               json_response(decorated_feature.as_json, 200)
-            rescue NameError, ArgumentError => exception
+            rescue NameError, ArgumentError
               json_error_response(:expression_invalid)
             end
           end

lib/flipper/cloud/configuration.rb

@@ -3,7 +3,6 @@
 require "flipper/adapters/http"
 require "flipper/adapters/poll"
 require "flipper/poller"
-require "flipper/adapters/memory"
 require "flipper/adapters/dual_write"
 require "flipper/adapters/sync/synchronizer"
 require "flipper/cloud/telemetry"

lib/flipper/export.rb

@@ -1,5 +1,3 @@
-require "flipper/adapters/memory"
-
 module Flipper
   class Export
     attr_reader :contents, :format, :version

lib/flipper/expressions/all.rb

@@ -1,5 +1,3 @@
-require "flipper/expression"
-
 module Flipper
   module Expressions
     class All

lib/flipper/feature.rb

@@ -100,9 +100,14 @@ def clear
     #
     # Returns true if enabled, false if not.
     def enabled?(*actors)
-      # Allows null object pattern. See PR for more.
-      # https://github.com/flippercloud/flipper/pull/887
-      actors = actors.flatten.reject(&:nil?).map { |actor| Types::Actor.wrap(actor) }
+      actors = Array(actors).
+        # Avoids to_ary warning that happens when passing DelegateClass of an 
+        # ActiveRecord object and using flatten here. This is tested in 
+        # spec/flipper/model/active_record_spec.rb.
+        flat_map { |actor| actor.is_a?(Array) ? actor : [actor] }. 
+        # Allows null object pattern. See PR for more. https://github.com/flippercloud/flipper/pull/887
+        reject(&:nil?). 
+        map { |actor| Types::Actor.wrap(actor) }
       actors = nil if actors.empty?
 
       # thing is left for backwards compatibility

lib/flipper/instrumentation/log_subscriber.rb

@@ -53,7 +53,6 @@ def adapter_operation(event)
 
         feature_name = event.payload[:feature_name]
         adapter_name = event.payload[:adapter_name]
-        gate_name = event.payload[:gate_name]
         operation = event.payload[:operation]
         result = event.payload[:result]
 

lib/flipper/instrumentation/statsd.rb

@@ -2,5 +2,7 @@
 require 'active_support/notifications'
 require 'flipper/instrumentation/statsd_subscriber'
 
-ActiveSupport::Notifications.subscribe /\.flipper$/,
-                                       Flipper::Instrumentation::StatsdSubscriber
+ActiveSupport::Notifications.subscribe(
+  /\.flipper$/,
+  Flipper::Instrumentation::StatsdSubscriber
+)

lib/flipper/instrumentation/subscriber.rb

@@ -42,7 +42,6 @@ def update
       # Private
       def update_feature_operation_metrics
         feature_name = @payload[:feature_name]
-        gate_name = @payload[:gate_name]
         operation = strip_trailing_question_mark(@payload[:operation])
         result = @payload[:result]
 
@@ -64,9 +63,6 @@ def update_feature_operation_metrics
       def update_adapter_operation_metrics
         adapter_name = @payload[:adapter_name]
         operation = @payload[:operation]
-        result = @payload[:result]
-        value = @payload[:value]
-        key = @payload[:key]
 
         update_timer "flipper.adapter.#{adapter_name}.#{operation}"
       end

lib/flipper/poller.rb

@@ -59,10 +59,10 @@ def stop
     def run
       loop do
         sleep jitter
-        start = Concurrent.monotonic_time
+
         begin
           sync
-        rescue => exception
+        rescue
           # you can instrument these using poller.flipper
         end
 

lib/flipper/ui/configuration.rb

@@ -5,8 +5,8 @@ module UI
     class Configuration
       attr_reader :delete
 
-      attr_accessor :banner_text,
-                    :banner_class
+      attr_accessor :banner_text
+      attr_reader :banner_class
 
       # Public: Is the UI in read only mode or not. Default is false. This
       # supersedes all other write-related options such as

lib/flipper/ui/views/layout.erb

@@ -1,7 +1,7 @@
 <!DOCTYPE html>
 <html lang="en">
   <head>
-    <title><%= @page_title ? "#{@page_title} // " : "" %>Flipper</title>
+    <title><%= defined?(@page_title) ? "#{@page_title} // " : "" %>Flipper</title>
     <meta charset="utf-8">
     <meta http-equiv="X-UA-Compatible" content="IE=edge">
     <meta name="viewport" content="width=device-width, initial-scale=1">

spec/flipper/adapters/active_record_spec.rb

@@ -9,9 +9,10 @@
 
   before(:all) do
     # Eval migration template so we can run migration against each database
-    migration = ERB.new(File.read(File.join(File.dirname(__FILE__), '../../../lib/generators/flipper/templates/migration.erb')))
+    template_path = File.join(File.dirname(__FILE__), '../../../lib/generators/flipper/templates/migration.erb')
+    migration = ERB.new(File.read(template_path))
     migration_version = "[#{ActiveRecord::VERSION::MAJOR}.#{ActiveRecord::VERSION::MINOR}]"
-    eval migration.result(binding) # defines CreateFlipperTables
+    eval migration.result_with_hash(migration_version: migration_version) # defines CreateFlipperTables
   end
 
   [
@@ -52,8 +53,10 @@
         end
 
         before(:each) do
-          ActiveRecord::Tasks::DatabaseTasks.purge(config)
-          CreateFlipperTables.migrate(:up)
+          silence do
+            ActiveRecord::Tasks::DatabaseTasks.purge(config)
+            CreateFlipperTables.migrate(:up)
+          end
         end
 
         after(:all) do

spec/flipper/adapters/http_spec.rb

@@ -28,6 +28,7 @@
       end
 
       before :all do
+        @started = false
         dir = FlipperRoot.join('tmp').tap(&:mkpath)
         log_path = dir.join('flipper_adapters_http_spec.log')
         @pstore_file = dir.join('flipper.pstore')

spec/flipper/adapters/mongo_spec.rb

@@ -30,7 +30,7 @@
     Flipper.configuration = nil
     Flipper.instance = nil
 
-    load 'flipper/adapters/mongo.rb'
+    silence { load 'flipper/adapters/mongo.rb' }
 
     ENV["MONGO_URL"] = ENV.fetch("MONGO_URL", "mongodb://127.0.0.1:27017/testing")
     expect(Flipper.adapter.adapter).to be_a(Flipper::Adapters::Mongo)

spec/flipper/api/action_spec.rb

@@ -74,7 +74,7 @@ def delete
         response = catch(:halt) do
           action.json_error_response(:feature_not_found)
         end
-        status, headers, body = response
+        _, headers, body = response
         parsed_body = JSON.parse(body[0])
 
         expect(headers[Rack::CONTENT_TYPE]).to eq('application/json')
@@ -88,7 +88,7 @@ def delete
         response = catch(:halt) do
           action.json_error_response(:group_not_registered)
         end
-        status, headers, body = response
+        _, headers, body = response
         parsed_body = JSON.parse(body[0])
 
         expect(headers[Rack::CONTENT_TYPE]).to eq('application/json')

spec/flipper/cloud/dsl_spec.rb

@@ -45,7 +45,7 @@
     end
 
     let(:cloud_configuration) do
-      cloud_configuration = Flipper::Cloud::Configuration.new({
+      Flipper::Cloud::Configuration.new({
         token: "asdf",
         sync_secret: "tasty",
         local_adapter: local_adapter

spec/flipper/cloud/telemetry_spec.rb

@@ -45,7 +45,7 @@
 
     expect(telemetry.interval).to eq(120)
     expect(telemetry.timer.execution_interval).to eq(120)
-    expect(stub).to have_been_requested
+    expect(stub).to have_been_requested.at_least_once
   end
 
   it "phones home and requests shutdown if telemetry-shutdown header is true" do
@@ -67,7 +67,7 @@
       result: true,
     })
     telemetry.stop
-    expect(stub).to have_been_requested
+    expect(stub).to have_been_requested.at_least_once
     expect(output.string).to match(/action=telemetry_shutdown message=The server has requested that telemetry be shut down./)
   end
 
@@ -90,7 +90,7 @@
       result: true,
     })
     telemetry.stop
-    expect(stub).to have_been_requested
+    expect(stub).to have_been_requested.at_least_once
     expect(output.string).not_to match(/action=telemetry_shutdown message=The server has requested that telemetry be shut down./)
   end
 

spec/flipper/dsl_spec.rb

@@ -225,9 +225,6 @@
 
   describe '#enable_group/disable_group' do
     it 'enables and disables the feature for group' do
-      actor = Flipper::Actor.new(5)
-      group = Flipper.register(:fives) { |actor| actor.flipper_id == 5 }
-
       expect(subject[:stats].groups_value).to be_empty
       subject.enable_group(:stats, :fives)
       expect(subject[:stats].groups_value).to eq(Set['fives'])

spec/flipper/feature_spec.rb

@@ -215,7 +215,7 @@ def actor.nil?
 
     it 'is recorded for enable' do
       actor = Flipper::Types::Actor.new(Flipper::Actor.new('1'))
-      gate = subject.gate_for(actor)
+      subject.gate_for(actor)
 
       subject.enable(actor)
 
@@ -230,7 +230,7 @@ def actor.nil?
 
     it 'always instruments flipper type instance for enable' do
       actor = Flipper::Actor.new('1')
-      gate = subject.gate_for(actor)
+      subject.gate_for(actor)
 
       subject.enable(actor)
 
@@ -241,7 +241,6 @@ def actor.nil?
 
     it 'is recorded for disable' do
       thing = Flipper::Types::Boolean.new
-      gate = subject.gate_for(thing)
 
       subject.disable(thing)
 
@@ -286,7 +285,6 @@ def actor.nil?
 
     it 'always instruments flipper type instance for disable' do
       actor = Flipper::Actor.new('1')
-      gate = subject.gate_for(actor)
 
       subject.disable(actor)
 
@@ -729,7 +727,6 @@ def actor.nil?
     context "with expression instance" do
       it "updates gate values to equal expression or clears expression" do
         expression = Flipper.property(:plan).eq("basic")
-        other_expression = Flipper.property(:age).gte(21)
         expect(subject.gate_values.expression).to be(nil)
         subject.enable_expression(expression)
         expect(subject.gate_values.expression).to eq(expression.value)
@@ -741,7 +738,6 @@ def actor.nil?
     context "with Hash" do
       it "updates gate values to equal expression or clears expression" do
         expression = Flipper.property(:plan).eq("basic")
-        other_expression = Flipper.property(:age).gte(21)
         expect(subject.gate_values.expression).to be(nil)
         subject.enable_expression(expression.value)
         expect(subject.gate_values.expression).to eq(expression.value)
@@ -1098,8 +1094,6 @@ def actor.nil?
   describe '#enable_group/disable_group' do
     context 'with symbol group name' do
       it 'updates the gate values to include the group' do
-        actor = Flipper::Actor.new(5)
-        group = Flipper.register(:five_only) { |actor| actor.flipper_id == 5 }
         expect(subject.gate_values.groups).to be_empty
         subject.enable_group(:five_only)
         expect(subject.gate_values.groups).to eq(Set['five_only'])
@@ -1110,8 +1104,6 @@ def actor.nil?
 
     context 'with string group name' do
       it 'updates the gate values to include the group' do
-        actor = Flipper::Actor.new(5)
-        group = Flipper.register(:five_only) { |actor| actor.flipper_id == 5 }
         expect(subject.gate_values.groups).to be_empty
         subject.enable_group('five_only')
         expect(subject.gate_values.groups).to eq(Set['five_only'])
@@ -1122,7 +1114,6 @@ def actor.nil?
 
     context 'with group instance' do
       it 'updates the gate values for the group' do
-        actor = Flipper::Actor.new(5)
         group = Flipper.register(:five_only) { |actor| actor.flipper_id == 5 }
         expect(subject.gate_values.groups).to be_empty
         subject.enable_group(group)

spec/flipper/instrumentation/log_subscriber_spec.rb

@@ -1,4 +1,5 @@
 require 'logger'
+require 'active_support/core_ext/object/blank'
 require 'flipper/instrumentation/log_subscriber'
 require 'flipper/adapters/instrumented'
 

spec/flipper/instrumentation/statsd_subscriber_spec.rb

@@ -18,7 +18,7 @@
     Flipper.new(adapter, instrumenter: ActiveSupport::Notifications)
   end
 
-  let(:user) { user = Flipper::Actor.new('1') }
+  let(:user) { Flipper::Actor.new('1') }
 
   before do
     described_class.client = statsd_client

spec/flipper/middleware/memoizer_spec.rb

@@ -80,7 +80,7 @@
   context 'with preload: true' do
     let(:app) do
       # ensure scoped for builder block, annoying...
-      instance = flipper
+      flipper
       middleware = described_class
 
       Rack::Builder.new do
@@ -141,7 +141,7 @@
   context 'with preload specific' do
     let(:app) do
       # ensure scoped for builder block, annoying...
-      instance = flipper
+      flipper
       middleware = described_class
 
       Rack::Builder.new do
@@ -266,7 +266,7 @@
   context 'with multiple instances' do
     let(:app) do
       # ensure scoped for builder block, annoying...
-      instance = flipper
+      flipper
       middleware = described_class
 
       Rack::Builder.new do
@@ -316,7 +316,7 @@ def get(uri, params = {}, env = {}, &block)
   context 'with flipper setup in env' do
     let(:app) do
       # ensure scoped for builder block, annoying...
-      instance = flipper
+      flipper
       middleware = described_class
 
       Rack::Builder.new do
@@ -460,7 +460,6 @@ def get(uri, params = {}, env = {}, &block)
       cache.clear
       cached = Flipper::Adapters::ActiveSupportCacheStore.new(logged_memory, cache)
       logged_cached = Flipper::Adapters::OperationLogger.new(cached)
-      memo = {}
       flipper = Flipper.new(logged_cached)
       flipper[:stats].enable
       flipper[:shiny].enable