Merge pull request #712 from airblade/lovely_linter

Lovely linter

.rubocop_todo.yml

@@ -1,77 +1,6 @@
 # Remove these configuration records
 # one by one as the offenses are removed from the code base.
 
-# Offense count: 3
-Lint/AmbiguousRegexpLiteral:
-  Exclude:
-    - 'test/unit/model_test.rb'
-
-# Offense count: 2
-# Cop supports --auto-correct.
-Lint/DeprecatedClassMethods:
-  Exclude:
-    - 'spec/rails_helper.rb'
-    - 'test/test_helper.rb'
-
-# Offense count: 3
-Lint/HandleExceptions:
-  Exclude:
-    - 'lib/paper_trail.rb'
-    - 'spec/spec_helper.rb'
-    - 'test/test_helper.rb'
-
-# Offense count: 2
-Lint/IneffectiveAccessModifier:
-  Exclude:
-    - 'lib/paper_trail.rb'
-
-# Offense count: 1
-Lint/Loop:
-  Exclude:
-    - 'test/functional/thread_safety_test.rb'
-
-# Offense count: 3
-Lint/UnderscorePrefixedVariableName:
-  Exclude:
-    - 'lib/paper_trail/cleaner.rb'
-    - 'lib/paper_trail/has_paper_trail.rb'
-    - 'lib/paper_trail/reifier.rb'
-
-# Offense count: 12
-# Cop supports --auto-correct.
-# Configuration parameters: IgnoreEmptyBlocks.
-Lint/UnusedBlockArgument:
-  Exclude:
-    - 'lib/paper_trail/cleaner.rb'
-    - 'lib/paper_trail/has_paper_trail.rb'
-    - 'test/custom_json_serializer.rb'
-    - 'test/unit/model_test.rb'
-    - 'test/unit/serializer_test.rb'
-    - 'test/unit/serializers/mixin_json_test.rb'
-    - 'test/unit/serializers/mixin_yaml_test.rb'
-
-# Offense count: 1
-# Cop supports --auto-correct.
-# Configuration parameters: AllowUnusedKeywordArguments, IgnoreEmptyMethods.
-Lint/UnusedMethodArgument:
-  Exclude:
-    - 'lib/paper_trail/has_paper_trail.rb'
-
-# Offense count: 1
-Lint/UselessAccessModifier:
-  Exclude:
-    - 'lib/paper_trail.rb'
-
-# Offense count: 1
-Lint/UselessAssignment:
-  Exclude:
-    - 'test/functional/controller_test.rb'
-
-# Offense count: 1
-Lint/Void:
-  Exclude:
-    - 'test/unit/cleaner_test.rb'
-
 # Offense count: 19
 Metrics/AbcSize:
   Max: 159

lib/paper_trail.rb

@@ -13,139 +13,160 @@ end
 module PaperTrail
   extend PaperTrail::Cleaner
 
-  # Switches PaperTrail on or off.
-  def self.enabled=(value)
-    PaperTrail.config.enabled = value
-  end
-
-  # Returns `true` if PaperTrail is on, `false` otherwise.
-  # PaperTrail is enabled by default.
-  def self.enabled?
-    !!PaperTrail.config.enabled
-  end
-
-  def self.serialized_attributes?
-    ActiveSupport::Deprecation.warn(
-      "PaperTrail.serialized_attributes? is deprecated without replacement " +
-        "and always returns false."
-    )
-    false
-  end
-
-  # Sets whether PaperTrail is enabled or disabled for the current request.
-  def self.enabled_for_controller=(value)
-    paper_trail_store[:request_enabled_for_controller] = value
-  end
-
-  # Returns `true` if PaperTrail is enabled for the request, `false` otherwise.
-  #
-  # See `PaperTrail::Rails::Controller#paper_trail_enabled_for_controller`.
-  def self.enabled_for_controller?
-    !!paper_trail_store[:request_enabled_for_controller]
-  end
-
-  # Sets whether PaperTrail is enabled or disabled for this model in the
-  # current request.
-  def self.enabled_for_model(model, value)
-    paper_trail_store[:"enabled_for_#{model}"] = value
-  end
-
-  # Returns `true` if PaperTrail is enabled for this model in the current
-  # request, `false` otherwise.
-  def self.enabled_for_model?(model)
-    !!paper_trail_store.fetch(:"enabled_for_#{model}", true)
-  end
-
-  # Set the field which records when a version was created.
-  def self.timestamp_field=(field_name)
-    PaperTrail.config.timestamp_field = field_name
-  end
-
-  # Returns the field which records when a version was created.
-  def self.timestamp_field
-    PaperTrail.config.timestamp_field
-  end
-
-  # Sets who is responsible for any changes that occur. You would normally use
-  # this in a migration or on the console, when working with models directly.
-  # In a controller it is set automatically to the `current_user`.
-  def self.whodunnit=(value)
-    paper_trail_store[:whodunnit] = value
-  end
-
-  # Returns who is reponsible for any changes that occur.
-  def self.whodunnit
-    paper_trail_store[:whodunnit]
-  end
-
-  # Sets any information from the controller that you want PaperTrail to
-  # store.  By default this is set automatically by a before filter.
-  def self.controller_info=(value)
-    paper_trail_store[:controller_info] = value
-  end
-
-  # Returns any information from the controller that you want
-  # PaperTrail to store.
-  #
-  # See `PaperTrail::Rails::Controller#info_for_paper_trail`.
-  def self.controller_info
-    paper_trail_store[:controller_info]
-  end
-
-  # Getter and Setter for PaperTrail Serializer
-  def self.serializer=(value)
-    PaperTrail.config.serializer = value
-  end
-
-  def self.serializer
-    PaperTrail.config.serializer
-  end
-
-  def self.active_record_protected_attributes?
-    @active_record_protected_attributes ||= ::ActiveRecord::VERSION::MAJOR < 4 ||
-      !!defined?(ProtectedAttributes)
-  end
-
-  def self.transaction?
-    ::ActiveRecord::Base.connection.open_transactions > 0
-  end
-
-  def self.transaction_id
-    paper_trail_store[:transaction_id]
-  end
-
-  def self.transaction_id=(id)
-    paper_trail_store[:transaction_id] = id
-  end
-
-  private
-
-  # Thread-safe hash to hold PaperTrail's data. Initializing with needed
-  # default values.
-  def self.paper_trail_store
-    RequestStore.store[:paper_trail] ||= { request_enabled_for_controller: true }
-  end
-
-  # Returns PaperTrail's configuration object.
-  def self.config
-    @config ||= PaperTrail::Config.instance
-    yield @config if block_given?
-    @config
-  end
-
   class << self
+    # Switches PaperTrail on or off.
+    # @api public
+    def enabled=(value)
+      PaperTrail.config.enabled = value
+    end
+
+    # Returns `true` if PaperTrail is on, `false` otherwise.
+    # PaperTrail is enabled by default.
+    # @api public
+    def enabled?
+      !!PaperTrail.config.enabled
+    end
+
+    def serialized_attributes?
+      ActiveSupport::Deprecation.warn(
+        "PaperTrail.serialized_attributes? is deprecated without replacement " +
+          "and always returns false."
+      )
+      false
+    end
+
+    # Sets whether PaperTrail is enabled or disabled for the current request.
+    # @api public
+    def enabled_for_controller=(value)
+      paper_trail_store[:request_enabled_for_controller] = value
+    end
+
+    # Returns `true` if PaperTrail is enabled for the request, `false` otherwise.
+    #
+    # See `PaperTrail::Rails::Controller#paper_trail_enabled_for_controller`.
+    # @api public
+    def enabled_for_controller?
+      !!paper_trail_store[:request_enabled_for_controller]
+    end
+
+    # Sets whether PaperTrail is enabled or disabled for this model in the
+    # current request.
+    # @api public
+    def enabled_for_model(model, value)
+      paper_trail_store[:"enabled_for_#{model}"] = value
+    end
+
+    # Returns `true` if PaperTrail is enabled for this model in the current
+    # request, `false` otherwise.
+    # @api public
+    def enabled_for_model?(model)
+      !!paper_trail_store.fetch(:"enabled_for_#{model}", true)
+    end
+
+    # Set the field which records when a version was created.
+    # @api public
+    def timestamp_field=(field_name)
+      PaperTrail.config.timestamp_field = field_name
+    end
+
+    # Returns the field which records when a version was created.
+    # @api public
+    def timestamp_field
+      PaperTrail.config.timestamp_field
+    end
+
+    # Sets who is responsible for any changes that occur. You would normally use
+    # this in a migration or on the console, when working with models directly.
+    # In a controller it is set automatically to the `current_user`.
+    # @api public
+    def whodunnit=(value)
+      paper_trail_store[:whodunnit] = value
+    end
+
+    # Returns who is reponsible for any changes that occur.
+    # @api public
+    def whodunnit
+      paper_trail_store[:whodunnit]
+    end
+
+    # Sets any information from the controller that you want PaperTrail to
+    # store.  By default this is set automatically by a before filter.
+    # @api public
+    def controller_info=(value)
+      paper_trail_store[:controller_info] = value
+    end
+
+    # Returns any information from the controller that you want
+    # PaperTrail to store.
+    #
+    # See `PaperTrail::Rails::Controller#info_for_paper_trail`.
+    # @api public
+    def controller_info
+      paper_trail_store[:controller_info]
+    end
+
+    # Getter and Setter for PaperTrail Serializer
+    # @api public
+    def serializer=(value)
+      PaperTrail.config.serializer = value
+    end
+
+    # @api public
+    def serializer
+      PaperTrail.config.serializer
+    end
+
+    # Returns a boolean indicating whether "protected attibutes" should be
+    # configured, e.g. attr_accessible, mass_assignment_sanitizer,
+    # whitelist_attributes, etc.
+    # @api public
+    def active_record_protected_attributes?
+      @active_record_protected_attributes ||= ::ActiveRecord::VERSION::MAJOR < 4 ||
+        !!defined?(ProtectedAttributes)
+    end
+
+    # @api public
+    def transaction?
+      ::ActiveRecord::Base.connection.open_transactions > 0
+    end
+
+    # @api public
+    def transaction_id
+      paper_trail_store[:transaction_id]
+    end
+
+    # @api public
+    def transaction_id=(id)
+      paper_trail_store[:transaction_id] = id
+    end
+
+    # Thread-safe hash to hold PaperTrail's data. Initializing with needed
+    # default values.
+    # @api private
+    def paper_trail_store
+      RequestStore.store[:paper_trail] ||= { request_enabled_for_controller: true }
+    end
+
+    # Returns PaperTrail's configuration object.
+    # @api private
+    def config
+      @config ||= PaperTrail::Config.instance
+      yield @config if block_given?
+      @config
+    end
+
     alias_method :configure, :config
   end
 end
 
-# Ensure `ProtectedAttributes` gem gets required if it is available before the
-# `Version` class gets loaded in.
+# If available, ensure that the `protected_attributes` gem is loaded
+# before the `Version` class.
 unless PaperTrail.active_record_protected_attributes?
   PaperTrail.send(:remove_instance_variable, :@active_record_protected_attributes)
   begin
     require 'protected_attributes'
-  rescue LoadError
-    # In case `ProtectedAttributes` gem is not available.
+  rescue LoadError # rubocop:disable Lint/HandleExceptions
+    # In case `protected_attributes` gem is not available.
   end
 end
 

lib/paper_trail/cleaner.rb

@@ -16,12 +16,12 @@ module PaperTrail
     #
     def clean_versions!(options = {})
       options = {:keeping => 1, :date => :all}.merge(options)
-      gather_versions(options[:item_id], options[:date]).each do |item_id, versions|
-        group_versions_by_date(versions).each do |date, _versions|
+      gather_versions(options[:item_id], options[:date]).each do |_item_id, item_versions|
+        group_versions_by_date(item_versions).each do |_date, date_versions|
           # Remove the number of versions we wish to keep from the collection
           # of versions prior to destruction.
-          _versions.pop(options[:keeping])
-          _versions.map(&:destroy)
+          date_versions.pop(options[:keeping])
+          date_versions.map(&:destroy)
         end
       end
     end

lib/paper_trail/has_paper_trail.rb

@@ -211,7 +211,9 @@ module PaperTrail
       end
 
       # Returns the objects (not Versions) as they were between the given times.
-      def versions_between(start_time, end_time, reify_options={})
+      # TODO: Either add support for the third argument, `_reify_options`, or
+      # add a deprecation warning if someone tries to use it.
+      def versions_between(start_time, end_time, _reify_options={})
         versions = send(self.class.versions_association_name).between(start_time, end_time)
         versions.collect { |version| version_at(version.send PaperTrail.timestamp_field) }
       end
@@ -384,9 +386,9 @@ module PaperTrail
       end
 
       def changes_for_paper_trail
-        _changes = changes.delete_if { |k,v| !notably_changed.include?(k) }
-        self.class.serialize_attribute_changes_for_paper_trail!(_changes)
-        _changes.to_hash
+        notable_changes = changes.delete_if { |k, _v| !notably_changed.include?(k) }
+        self.class.serialize_attribute_changes_for_paper_trail!(notable_changes)
+        notable_changes.to_hash
       end
 
       # Invoked via`after_update` callback for when a previous version is
@@ -469,7 +471,7 @@ module PaperTrail
 
       def merge_metadata(data)
         # First we merge the model-level metadata in `meta`.
-        paper_trail_options[:meta].each do |k,v|
+        paper_trail_options[:meta].each do |k, v|
           data[k] =
             if v.respond_to?(:call)
               v.call(self)
@@ -493,7 +495,7 @@ module PaperTrail
       def attributes_before_change
         attributes.tap do |prev|
           enums = self.respond_to?(:defined_enums) ? self.defined_enums : {}
-          attrs = changed_attributes.select { |k, v| self.class.column_names.include?(k) }
+          attrs = changed_attributes.select { |k, _v| self.class.column_names.include?(k) }
           attrs.each do |attr, before|
             before = enums[attr][before] if enums[attr]
             prev[attr] = before

lib/paper_trail/reifier.rb

@@ -49,10 +49,10 @@ module PaperTrail
           klass = class_name.constantize
           # The `dup` option always returns a new object, otherwise we should
           # attempt to look for the item outside of default scope(s).
-          if options[:dup] || (_item = klass.unscoped.find_by_id(version.item_id)).nil?
+          if options[:dup] || (item_found = klass.unscoped.find_by_id(version.item_id)).nil?
             model = klass.new
           elsif options[:unversioned_attributes] == :nil
-            model = _item
+            model = item_found
             # Look for attributes that exist in the model and not in this
             # version. These attributes should be set to nil.
             (model.attribute_names - attrs.keys).each { |k| attrs[k] = nil }

spec/rails_helper.rb

@@ -2,7 +2,7 @@
 ENV["RAILS_ENV"] ||= 'test'
 ENV["DB"] ||= 'sqlite'
 
-unless File.exists?(File.expand_path('../../test/dummy/config/database.yml', __FILE__))
+unless File.exist?(File.expand_path('../../test/dummy/config/database.yml', __FILE__))
   warn "WARNING: No database.yml detected for the dummy app, please run `rake prepare` first"
 end
 

spec/spec_helper.rb

@@ -1,8 +1,4 @@
-begin
-  require 'pry-nav'
-rescue LoadError
-  # It's OK, we don't include pry in e.g. gemfiles/3.0.gemfile
-end
+require 'pry-nav'
 
 # This file was generated by the `rspec --init` command. Conventionally, all
 # specs live under a `spec` directory, which RSpec adds to the `$LOAD_PATH`.

test/custom_json_serializer.rb

@@ -4,10 +4,10 @@ module CustomJsonSerializer
 
   def self.load(string)
     parsed_value = super(string)
-    parsed_value.is_a?(Hash) ? parsed_value.reject { |k,v| k.blank? || v.blank? } : parsed_value
+    parsed_value.is_a?(Hash) ? parsed_value.reject { |k, v| k.blank? || v.blank? } : parsed_value
   end
 
   def self.dump(object)
-    object.is_a?(Hash) ? super(object.reject { |k,v| v.nil? }) : super
+    object.is_a?(Hash) ? super(object.reject { |_k, v| v.nil? }) : super
   end
-end
+end

test/functional/controller_test.rb

@@ -35,7 +35,6 @@ class ControllerTest < ActionController::TestCase
     w = assigns(:widget)
     assert_equal 0, w.versions.length
     delete :destroy, params_wrapper({ id: w.id })
-    widget = assigns(:widget)
     assert_equal 0, PaperTrail::Version.with_item_keys('Widget', w.id).size
   end
 

test/functional/thread_safety_test.rb

@@ -1,15 +1,13 @@
 require 'test_helper'
 
 class ThreadSafetyTest < ActionController::TestCase
-  test "be thread safe when using #set_paper_trail_whodunnit" do
+  test "thread-safe when using #set_paper_trail_whodunnit" do
     blocked = true
 
     slow_thread = Thread.new do
       controller = TestController.new
       controller.send :set_paper_trail_whodunnit
-      begin
-        sleep 0.001
-      end while blocked
+      sleep 0.001 while blocked
       PaperTrail.whodunnit
     end
 
@@ -24,7 +22,7 @@ class ThreadSafetyTest < ActionController::TestCase
     assert_not_equal slow_thread.value, fast_thread.value
   end
 
-  test "be thread safe when using #without_versioning" do
+  test "thread-safe when using #without_versioning" do
     enabled = nil
 
     slow_thread = Thread.new do

test/test_helper.rb

@@ -1,13 +1,9 @@
-begin
-  require 'pry-nav'
-rescue LoadError
-  # It's OK, we don't include pry in e.g. gemfiles/3.0.gemfile
-end
+require 'pry-nav'
 
 ENV["RAILS_ENV"] = "test"
 ENV["DB"] ||= "sqlite"
 
-unless File.exists?(File.expand_path('../../test/dummy/config/database.yml', __FILE__))
+unless File.exist?(File.expand_path('../../test/dummy/config/database.yml', __FILE__))
   warn "WARNING: No database.yml detected for the dummy app, please run `rake prepare` first"
 end
 

test/unit/cleaner_test.rb

@@ -174,9 +174,9 @@ class PaperTrailCleanerTest < ActiveSupport::TestCase
       assert_equal 9, PaperTrail::Version.count
       @animals.each do |animal|
         assert_equal 3, animal.versions.size
-        animal.versions.each_cons(2) do |a,b|
-          a.created_at.to_date == b.created_at.to_date
-          a.custom_created_at.to_date != b.custom_created_at.to_date
+        animal.versions.each_cons(2) do |a, b|
+          assert_equal a.created_at.to_date, b.created_at.to_date
+          assert_not_equal a.custom_created_at.to_date, b.custom_created_at.to_date
         end
       end
     end

test/unit/model_test.rb

@@ -255,7 +255,7 @@ class HasPaperTrailModelTest < ActiveSupport::TestCase
       end
 
       should 'record the correct event' do
-        assert_match /create/i, @widget.versions.first.event
+        assert_match(/create/i, @widget.versions.first.event)
       end
 
       should 'be live' do
@@ -311,7 +311,7 @@ class HasPaperTrailModelTest < ActiveSupport::TestCase
         end
 
         should 'record the correct event' do
-          assert_match /update/i, @widget.versions.last.event
+          assert_match(/update/i, @widget.versions.last.event)
         end
 
         should 'have versions that are not live' do
@@ -323,11 +323,11 @@ class HasPaperTrailModelTest < ActiveSupport::TestCase
           # AR4 includes the `updated_at` column in changes for updates, which
           # is why we reject it from the right side of this assertion.
           last_obj_changes = @widget.versions.last.object_changes
-          actual = PaperTrail.serializer.load(last_obj_changes).reject { |k, v|
+          actual = PaperTrail.serializer.load(last_obj_changes).reject { |k, _v|
             k.to_sym == :updated_at
           }
           assert_equal ({'name' => ['Henry', 'Harry']}), actual
-          actual = @widget.versions.last.changeset.reject { |k, v|
+          actual = @widget.versions.last.changeset.reject { |k, _v|
             k.to_sym == :updated_at
           }
           assert_equal ({'name' => ['Henry', 'Harry']}), actual
@@ -428,7 +428,7 @@ class HasPaperTrailModelTest < ActiveSupport::TestCase
           end
 
           should 'record the correct event' do
-            assert_match /destroy/i, PaperTrail::Version.last.event
+            assert_match(/destroy/i, PaperTrail::Version.last.event)
           end
 
           should 'have three previous versions' do
@@ -1106,7 +1106,7 @@ class HasPaperTrailModelTest < ActiveSupport::TestCase
       end
 
       should 'version.changeset should be the same as record.changes was before the save' do
-        actual = @person.versions.last.changeset.delete_if { |key, val| key.to_sym == :id }
+        actual = @person.versions.last.changeset.delete_if { |k, _v| k.to_sym == :id }
         assert_equal @changes_before_save, actual
         actual = @person.versions.last.changeset[:time_zone].map(&:class)
         assert_equal [NilClass, ActiveSupport::TimeZone], actual

test/unit/serializers/mixin_json_test.rb

@@ -20,7 +20,7 @@ class MixinJsonTest < ActiveSupport::TestCase
     end
 
     should '`deserialize` JSON to Ruby, removing pairs with `blank` keys or values' do
-      assert_equal @hash.reject { |k,v| k.blank? || v.blank? },
+      assert_equal @hash.reject { |k, v| k.blank? || v.blank? },
         CustomJsonSerializer.load(@hash_as_json)
     end
   end
@@ -31,7 +31,7 @@ class MixinJsonTest < ActiveSupport::TestCase
     end
 
     should '`serialize` Ruby to JSON, removing pairs with `nil` values' do
-      assert_equal @hash.reject { |k,v| v.nil? }.to_json,
+      assert_equal @hash.reject { |_k, v| v.nil? }.to_json,
         CustomJsonSerializer.dump(@hash)
     end
   end

test/unit/serializers/mixin_yaml_test.rb

@@ -13,7 +13,7 @@ module CustomYamlSerializer
   end
 
   def self.dump(object)
-    object.is_a?(Hash) ? super(object.reject { |k,v| v.nil? }) : super
+    object.is_a?(Hash) ? super(object.reject { |_k, v| v.nil? }) : super
   end
 end
 
@@ -36,7 +36,7 @@ class MixinYamlTest < ActiveSupport::TestCase
     end
 
     should '`deserialize` YAML to Ruby, removing pairs with `blank` keys or values' do
-      assert_equal @hash.reject { |k,v| k.blank? || v.blank? },
+      assert_equal @hash.reject { |k, v| k.blank? || v.blank? },
         CustomYamlSerializer.load(@hash_as_yaml)
     end
   end
@@ -47,9 +47,8 @@ class MixinYamlTest < ActiveSupport::TestCase
     end
 
     should '`serialize` Ruby to YAML, removing pairs with `nil` values' do
-      assert_equal @hash.reject { |k,v| v.nil? }.to_yaml,
+      assert_equal @hash.reject { |_k, v| v.nil? }.to_yaml,
         CustomYamlSerializer.dump(@hash)
     end
   end
-
 end