Fix several RuboCop TODO items (#3478)

Handle the simple and straightforward fixes: - Lint/AmbiguousBlockAssociation - Lint/AssignmentInCondition - Lint/DuplicateMethods - Lint/RequireParentheses - Naming/HeredocDelimiterNaming
railsadminteam · Feb 27, 2022 · d370a2a · d370a2a
1 parent f92bdc5
commit d370a2a
Show file tree

Hide file tree

Showing 23 changed files with 76 additions and 106 deletions.
diff --git a/.rubocop_todo.yml b/.rubocop_todo.yml
@@ -6,48 +6,12 @@
 # Note that changes in the inspected code, or installation of new
 # versions of RuboCop, may require this file to be generated again.
 
-# Offense count: 1
-# Configuration parameters: IgnoredMethods.
-Lint/AmbiguousBlockAssociation:
-  Exclude:
-    - "spec/rails_admin/install_generator_spec.rb"
-
-# Offense count: 18
-# Configuration parameters: AllowSafeAssignment.
-Lint/AssignmentInCondition:
-  Exclude:
-    - "app/controllers/rails_admin/main_controller.rb"
-    - "app/helpers/rails_admin/application_helper.rb"
-    - "lib/rails_admin/abstract_model.rb"
-    - "lib/rails_admin/adapters/active_record.rb"
-    - "lib/rails_admin/config/actions/export.rb"
-    - "lib/rails_admin/config/actions/index.rb"
-    - "lib/rails_admin/config/actions/new.rb"
-    - "lib/rails_admin/config/fields/factories/active_storage.rb"
-    - "lib/rails_admin/config/fields/factories/association.rb"
-    - "lib/rails_admin/config/fields/factories/carrierwave.rb"
-    - "lib/rails_admin/config/fields/factories/devise.rb"
-    - "lib/rails_admin/config/fields/factories/dragonfly.rb"
-    - "lib/rails_admin/config/fields/factories/paperclip.rb"
-    - "lib/rails_admin/config/fields/types/datetime.rb"
-    - "spec/dummy_app/db/seeds.rb"
-
 # Offense count: 51
 # Configuration parameters: AllowedMethods.
 # AllowedMethods: enums
 Lint/ConstantDefinitionInBlock:
   Enabled: false
 
-# Offense count: 1
-Lint/DuplicateMethods:
-  Exclude:
-    - "lib/rails_admin/config.rb"
-
-# Offense count: 1
-Lint/RequireParentheses:
-  Exclude:
-    - "spec/spec_helper.rb"
-
 # Offense count: 1
 Lint/ReturnInVoidContext:
   Exclude:
@@ -71,18 +35,6 @@ Naming/AccessorMethodName:
     - "app/controllers/rails_admin/main_controller.rb"
     - "lib/rails_admin/abstract_model.rb"
 
-# Offense count: 6
-# Configuration parameters: ForbiddenDelimiters.
-# ForbiddenDelimiters: (?-mix:(^|\s)(EO[A-Z]{1}|END)(\s|$))
-Naming/HeredocDelimiterNaming:
-  Exclude:
-    - "app/controllers/rails_admin/main_controller.rb"
-    - "lib/rails_admin/adapters/mongoid.rb"
-    - "lib/rails_admin/config/fields/base.rb"
-    - "lib/rails_admin/engine.rb"
-    - "lib/rails_admin/extensions/paper_trail/auditing_adapter.rb"
-    - "spec/factories.rb"
-
 # Offense count: 2
 # Configuration parameters: EnforcedStyleForLeadingUnderscores.
 # SupportedStylesForLeadingUnderscores: disallowed, required, optional

diff --git a/app/controllers/rails_admin/main_controller.rb b/app/controllers/rails_admin/main_controller.rb
@@ -13,9 +13,8 @@ def bulk_action
 
     def list_entries(model_config = @model_config, auth_scope_key = :index, additional_scope = get_association_scope_from_params, pagination = !(params[:associated_collection] || params[:all] || params[:bulk_ids]))
       scope = model_config.scope
-      if auth_scope = @authorization_adapter&.query(auth_scope_key, model_config.abstract_model)
-        scope = scope.merge(auth_scope)
-      end
+      auth_scope = @authorization_adapter&.query(auth_scope_key, model_config.abstract_model)
+      scope = scope.merge(auth_scope) if auth_scope
       scope = scope.instance_eval(&additional_scope) if additional_scope
       get_collection(model_config, scope, pagination)
     end

diff --git a/lib/rails_admin/abstract_model.rb b/lib/rails_admin/abstract_model.rb
@@ -86,9 +86,8 @@ def each_associated_children(object)
       associations.each do |association|
         case association.type
         when :has_one
-          if child = object.send(association.name)
-            yield(association, [child])
-          end
+          child = object.send(association.name)
+          yield(association, [child]) if child
         when :has_many
           children = object.send(association.name)
           yield(association, Array.new(children))

diff --git a/lib/rails_admin/adapters/active_record.rb b/lib/rails_admin/adapters/active_record.rb
@@ -13,7 +13,8 @@ def new(params = {})
       end
 
       def get(id, scope = scoped)
-        return unless object = scope.where(primary_key => id).first
+        object = scope.where(primary_key => id).first
+        return unless object
 
         object.extend(ObjectExtension)
       end

diff --git a/lib/rails_admin/adapters/mongoid.rb b/lib/rails_admin/adapters/mongoid.rb
@@ -52,10 +52,10 @@ def all(options = {}, scope = nil)
         scope
       rescue NoMethodError => e
         if /page/.match?(e.message)
-          e = e.exception <<-EOM.gsub(/^\s{12}/, '')
+          e = e.exception <<-ERROR.gsub(/^\s{12}/, '')
             #{e.message}
             If you don't have kaminari-mongoid installed, add `gem 'kaminari-mongoid'` to your Gemfile.
-          EOM
+          ERROR
         end
         raise e
       end

diff --git a/lib/rails_admin/config.rb b/lib/rails_admin/config.rb
@@ -39,7 +39,7 @@ class << self
       attr_accessor :included_models
 
       # Fields to be hidden in show, create and update views
-      attr_accessor :default_hidden_fields
+      attr_reader :default_hidden_fields
 
       # Default items per page value used if a model level option has not
       # been configured

diff --git a/lib/rails_admin/config/actions/export.rb b/lib/rails_admin/config/actions/export.rb
@@ -14,7 +14,8 @@ class Export < RailsAdmin::Config::Actions::Base
 
         register_instance_option :controller do
           proc do
-            if format = params[:json] && :json || params[:csv] && :csv || params[:xml] && :xml
+            format = params[:json] && :json || params[:csv] && :csv || params[:xml] && :xml
+            if format
               request.format = format
               @schema = HashHelper.symbolize(params[:schema].slice(:except, :include, :methods, :only).permit!.to_h) if params[:schema] # to_json and to_xml expect symbols for keys AND values.
               @objects = list_entries(@model_config, :export)

diff --git a/lib/rails_admin/config/actions/index.rb b/lib/rails_admin/config/actions/index.rb
@@ -20,7 +20,8 @@ class Index < RailsAdmin::Config::Actions::Base
 
         register_instance_option :breadcrumb_parent do
           parent_model = bindings[:abstract_model].try(:config).try(:parent)
-          if am = parent_model && RailsAdmin.config(parent_model).try(:abstract_model)
+          am = parent_model && RailsAdmin.config(parent_model).try(:abstract_model)
+          if am
             [:index, am]
           else
             [:dashboard]

diff --git a/lib/rails_admin/config/actions/new.rb b/lib/rails_admin/config/actions/new.rb
@@ -21,7 +21,8 @@ class New < RailsAdmin::Config::Actions::Base
               @authorization_adapter&.attributes_for(:new, @abstract_model)&.each do |name, value|
                 @object.send("#{name}=", value)
               end
-              if object_params = params[@abstract_model.param_key]
+              object_params = params[@abstract_model.param_key]
+              if object_params
                 sanitize_params_for!(request.xhr? ? :modal : :create)
                 @object.assign_attributes(@object.attributes.merge(object_params.to_h))
               end

diff --git a/lib/rails_admin/config/fields/base.rb b/lib/rails_admin/config/fields/base.rb
@@ -301,14 +301,14 @@ def type
         def value
           bindings[:object].safe_send(name)
         rescue NoMethodError => e
-          raise e.exception <<-EOM.gsub(/^\s{10}/, '')
+          raise e.exception <<-ERROR.gsub(/^\s{10}/, '')
           #{e.message}
           If you want to use a RailsAdmin virtual field(= a field without corresponding instance method),
           you should declare 'formatted_value' in the field definition.
             field :#{name} do
               formatted_value{ bindings[:object].call_some_method }
             end
-          EOM
+          ERROR
         end
 
         # Reader for nested attributes

diff --git a/lib/rails_admin/config/fields/factories/active_storage.rb b/lib/rails_admin/config/fields/factories/active_storage.rb
@@ -16,7 +16,8 @@
         ["#{name}_attachment".to_sym, "#{name}_blob".to_sym]
       end
     children_fields = associations.map do |child_name|
-      next unless child_association = parent.abstract_model.associations.detect { |p| p.name.to_sym == child_name }
+      child_association = parent.abstract_model.associations.detect { |p| p.name.to_sym == child_name }
+      next unless child_association
 
       child_field = fields.detect { |f| f.name == child_name } || RailsAdmin::Config::Fields.default_factory.call(parent, child_association, fields)
       child_field.hide unless field == child_field

diff --git a/lib/rails_admin/config/fields/factories/association.rb b/lib/rails_admin/config/fields/factories/association.rb
@@ -3,7 +3,8 @@
 require 'rails_admin/config/fields/types/belongs_to_association'
 
 RailsAdmin::Config::Fields.register_factory do |parent, properties, fields|
-  if association = parent.abstract_model.associations.detect { |a| a.foreign_key == properties.name && %i[belongs_to has_and_belongs_to_many].include?(a.type) }
+  association = parent.abstract_model.associations.detect { |a| a.foreign_key == properties.name && %i[belongs_to has_and_belongs_to_many].include?(a.type) }
+  if association
     field = RailsAdmin::Config::Fields::Types.load("#{association.polymorphic? ? :polymorphic : association.type}_association").new(parent, association.name, association)
     fields << field
 
@@ -15,9 +16,8 @@
                            end.collect { |k| association.send(k) }.compact
 
     parent.abstract_model.properties.select { |p| possible_field_names.include? p.name }.each do |column|
-      unless child_field = fields.detect { |f| f.name.to_s == column.name.to_s }
-        child_field = RailsAdmin::Config::Fields.default_factory.call(parent, column, fields)
-      end
+      child_field = fields.detect { |f| f.name.to_s == column.name.to_s }
+      child_field ||= RailsAdmin::Config::Fields.default_factory.call(parent, column, fields)
       child_columns << child_field
     end
 

diff --git a/lib/rails_admin/config/fields/factories/carrierwave.rb b/lib/rails_admin/config/fields/factories/carrierwave.rb
@@ -12,7 +12,8 @@
     fields << field
     children_fields = []
     columns.each do |children_column_name|
-      next unless child_properties = parent.abstract_model.properties.detect { |p| p.name.to_s == children_column_name.to_s }
+      child_properties = parent.abstract_model.properties.detect { |p| p.name.to_s == children_column_name.to_s }
+      next unless child_properties
 
       children_field = fields.detect { |f| f.name == children_column_name } || RailsAdmin::Config::Fields.default_factory.call(parent, child_properties, fields)
       children_field.hide unless field == children_field

diff --git a/lib/rails_admin/config/fields/factories/devise.rb b/lib/rails_admin/config/fields/factories/devise.rb
@@ -12,7 +12,8 @@
       properties = parent.abstract_model.properties.detect { |p| ext == p.name }
       next unless properties
 
-      unless field = fields.detect { |f| f.name == ext }
+      field = fields.detect { |f| f.name == ext }
+      unless field
         RailsAdmin::Config::Fields.default_factory.call(parent, properties, fields)
         field = fields.last
       end

diff --git a/lib/rails_admin/config/fields/factories/dragonfly.rb b/lib/rails_admin/config/fields/factories/dragonfly.rb
@@ -9,7 +9,8 @@
     children_fields = []
     extensions.each do |ext|
       children_column_name = "#{attachment_name}_#{ext}".to_sym
-      next unless child_properties = parent.abstract_model.properties.detect { |p| p.name.to_s == children_column_name.to_s }
+      child_properties = parent.abstract_model.properties.detect { |p| p.name.to_s == children_column_name.to_s }
+      next unless child_properties
 
       children_field = fields.detect { |f| f.name == children_column_name } || RailsAdmin::Config::Fields.default_factory.call(parent, child_properties, fields)
       children_field.hide

diff --git a/lib/rails_admin/config/fields/factories/paperclip.rb b/lib/rails_admin/config/fields/factories/paperclip.rb
@@ -10,7 +10,8 @@
     children_fields = []
     extensions.each do |ext|
       children_column_name = "#{attachment_name}_#{ext}".to_sym
-      next unless child_properties = parent.abstract_model.properties.detect { |p| p.name.to_s == children_column_name.to_s }
+      child_properties = parent.abstract_model.properties.detect { |p| p.name.to_s == children_column_name.to_s }
+      next unless child_properties
 
       children_field = fields.detect { |f| f.name == children_column_name } || RailsAdmin::Config::Fields.default_factory.call(parent, child_properties, fields)
       children_field.hide

diff --git a/lib/rails_admin/config/fields/types/datetime.rb b/lib/rails_admin/config/fields/types/datetime.rb
@@ -58,7 +58,8 @@ def parse_input(params)
           end
 
           register_instance_option :formatted_value do
-            if time = (value || default_value)
+            time = (value || default_value)
+            if time
               ::I18n.l(time, format: strftime_format)
             else
               ''.html_safe

diff --git a/lib/rails_admin/engine.rb b/lib/rails_admin/engine.rb
@@ -59,14 +59,14 @@ class Engine < Rails::Engine
       unless missing.empty? && has_session_store
         configs = missing.map { |m| "config.middleware.use #{m}" }
         configs << "config.middleware.use #{app.config.session_store.try(:name) || 'ActionDispatch::Session::CookieStore'}, #{app.config.session_options}" unless has_session_store
-        raise <<~EOM
+        raise <<~ERROR
           Required middlewares for RailsAdmin are not added
           To fix this, add
 
             #{configs.join("\n  ")}
 
           to config/application.rb.
-        EOM
+        ERROR
       end
 
       RailsAdmin::Config.initialize!

diff --git a/lib/rails_admin/extensions/paper_trail/auditing_adapter.rb b/lib/rails_admin/extensions/paper_trail/auditing_adapter.rb
@@ -49,7 +49,7 @@ class AuditingAdapter
           created_at: :created_at,
           message: :event,
         }.freeze
-        E_VERSION_MODEL_NOT_SET = <<-EOS.strip_heredoc.freeze
+        E_VERSION_MODEL_NOT_SET = <<-ERROR.strip_heredoc.freeze
           Please set up PaperTrail's version model explicitly.
 
               config.audit_with :paper_trail, 'User', 'PaperTrail::Version'
@@ -58,7 +58,7 @@ class AuditingAdapter
           (https://github.com/paper-trail-gem/paper_trail#6a-custom-version-classes)
           that configuration will take precedence over what you specify in
           `audit_with`.
-        EOS
+        ERROR
 
         def self.setup
           raise 'PaperTrail not found' unless defined?(::PaperTrail)

diff --git a/spec/dummy_app/db/seeds.rb b/spec/dummy_app/db/seeds.rb
@@ -16,15 +16,18 @@
 results = connection.get('/sferik/mlb/e5b9384fc388f34ec5baca291343864135dcb0fe/cache/teams.json').body
 
 MLB::Team.send(:results_to_team, results).each do |mlb_team|
-  unless league = league_model.where(name: mlb_team.league).first
+  league = league_model.where(name: mlb_team.league).first
+  unless league
     league = league_model.model.new(name: mlb_team.league)
     league.save!
   end
-  unless division = division_model.where(name: mlb_team.division).first
+  division = division_model.where(name: mlb_team.division).first
+  unless division
     division = division_model.model.new(name: mlb_team.division, league: league)
     division.save!
   end
-  unless team = team_model.where(name: mlb_team.name).first
+  team = team_model.where(name: mlb_team.name).first
+  unless team
     team = team_model.model.new(name: mlb_team.name, logo_url: mlb_team.logo_url, manager: mlb_team.manager || 'None', ballpark: mlb_team.ballpark, mascot: mlb_team.mascot, founded: mlb_team.founded, wins: mlb_team.wins, losses: mlb_team.losses, win_percentage: format('%.3f', (mlb_team.wins.to_f / (mlb_team.wins + mlb_team.losses))).to_f, division: division)
     team.save!
   end

diff --git a/spec/factories.rb b/spec/factories.rb
@@ -56,14 +56,14 @@
 
   factory :comment do
     sequence(:content) do |n|
-      <<-EOF
+      <<-LOREM_IPSUM
         Lorém --#{n}-- ipsum dolor sit amet, consectetur adipisicing elit, sed do eiusmod
         tempor incididunt ut labore et dolore magna aliqua. Ut enim ad minim veniam,
         quis nostrud exercitation ullamco laboris nisi ut aliquip ex ea commodo
         consequat. Duis aute irure dolor in reprehenderit in voluptate velit esse
         cillum dolore eu fugiat nulla pariatur. Excepteur sint occaecat cupidatat non
         proident, sunt in culpa qui officia deserunt mollit anim id est laborum."
-      EOF
+      LOREM_IPSUM
     end
 
     factory :comment_confirmed, class: Comment::Confirmed do

diff --git a/spec/rails_admin/install_generator_spec.rb b/spec/rails_admin/install_generator_spec.rb
@@ -25,32 +25,34 @@
     Dir.chdir(destination_root) do
       run_generator
     end
-    expect(destination_root).to have_structure {
-      directory 'config' do
-        directory 'initializers' do
-          file 'rails_admin.rb' do
-            contains 'RailsAdmin.config'
-            contains 'asset_source ='
+    expect(destination_root).to(
+      have_structure do
+        directory 'config' do
+          directory 'initializers' do
+            file 'rails_admin.rb' do
+              contains 'RailsAdmin.config'
+              contains 'asset_source ='
+            end
+          end
+          file 'routes.rb' do
+            contains "mount RailsAdmin::Engine => '/admin', as: 'rails_admin'"
           end
         end
-        file 'routes.rb' do
-          contains "mount RailsAdmin::Engine => '/admin', as: 'rails_admin'"
-        end
-      end
-      case CI_ASSET
-      when :webpacker
-        file 'app/javascript/packs/rails_admin.js' do
-          contains 'import "rails_admin/src/rails_admin/base"'
-        end
-        file 'app/javascript/stylesheets/rails_admin.scss' do
-          contains '@import "~rails_admin/src/rails_admin/styles/base"'
-        end
-      when :sprockets
-        file 'Gemfile' do
-          contains 'sassc-rails'
+        case CI_ASSET
+        when :webpacker
+          file 'app/javascript/packs/rails_admin.js' do
+            contains 'import "rails_admin/src/rails_admin/base"'
+          end
+          file 'app/javascript/stylesheets/rails_admin.scss' do
+            contains '@import "~rails_admin/src/rails_admin/styles/base"'
+          end
+        when :sprockets
+          file 'Gemfile' do
+            contains 'sassc-rails'
+          end
         end
-      end
-    }
+      end,
+    )
   end
 
   it 'inserts asset_source option to RailsAdmin Initializer' do

diff --git a/spec/spec_helper.rb b/spec/spec_helper.rb
@@ -101,7 +101,12 @@ def password_digest(password)
   end
 
   config.before do |example|
-    DatabaseCleaner.strategy = CI_ORM == :mongoid || example.metadata[:js] ? :deletion : :transaction
+    DatabaseCleaner.strategy =
+      if CI_ORM == :mongoid || example.metadata[:js]
+        :deletion
+      else
+        :transaction
+      end
 
     DatabaseCleaner.start
     RailsAdmin::Config.reset