From 1e87718222686d9c58a97453db471f556cc28a1d Mon Sep 17 00:00:00 2001 From: Jesse Szwedko Date: Mon, 28 Aug 2017 12:34:57 -0700 Subject: [PATCH] Lint with up-to-date rubocop Matching version bump of rubocop --- .rubocop.yml | 26 +++++--- Gemfile | 13 ++-- Rakefile | 2 +- app/controllers/info_window_controller.rb | 2 +- app/controllers/reminders_controller.rb | 2 +- config/initializers/airbrake.rb | 2 +- ...0005_create_rails_admin_histories_table.rb | 2 +- lib/thing_importer.rb | 63 ++++++++++--------- .../info_window_controller_test.rb | 2 +- test/lib/thing_importer_test.rb | 26 ++++---- test/mailers/thing_mailer_test.rb | 10 +-- 11 files changed, 79 insertions(+), 71 deletions(-) diff --git a/.rubocop.yml b/.rubocop.yml index 90432890..37245760 100644 --- a/.rubocop.yml +++ b/.rubocop.yml @@ -1,13 +1,23 @@ -Rails: +Rails: Enabled: true AllCops: + TargetRailsVersion: 4.0 Exclude: - 'bin/*' - 'db/schema.rb' - 'db/seeds.rb' - 'vendor/bundle/**/*' +Layout/DotPosition: + EnforcedStyle: trailing + +Layout/SpaceInsideHashLiteralBraces: + EnforcedStyle: no_space + +Layout/AccessModifierIndentation: + EnforcedStyle: outdent + Metrics/AbcSize: Exclude: - 'app/controllers/passwords_controller.rb' @@ -15,6 +25,11 @@ Metrics/AbcSize: - 'app/controllers/users_controller.rb' - 'lib/thing_importer.rb' +Metrics/BlockLength: + Exclude: + - 'config/initializers/*' + ExcludedMethods: ['test'] + Metrics/BlockNesting: Max: 2 @@ -33,9 +48,6 @@ Metrics/ParameterLists: Max: 4 CountKeywordArgs: true -Style/AccessModifierIndentation: - EnforcedStyle: outdent - Style/CollectionMethods: PreferredMethods: map: 'collect' @@ -46,15 +58,9 @@ Style/CollectionMethods: Style/Documentation: Enabled: false -Style/DotPosition: - EnforcedStyle: trailing - Style/DoubleNegation: Enabled: false -Style/SpaceInsideHashLiteralBraces: - EnforcedStyle: no_space - Style/TrailingCommaInArguments: EnforcedStyleForMultiline: 'comma' diff --git a/Gemfile b/Gemfile index 972bd4bf..f015dddf 100644 --- a/Gemfile +++ b/Gemfile @@ -1,8 +1,6 @@ source 'https://rubygems.org' ruby '2.2.3' -gem 'dotenv-rails', groups: [:development, :test] -gem 'rails', '~> 4.2.4' gem 'airbrake', '~> 5.2' gem 'devise', '~> 3.0' gem 'geokit', '~> 1.0' @@ -11,11 +9,15 @@ gem 'http_accept_language', '~> 2.0' gem 'local_time', '~> 1.0' gem 'obscenity', '~> 1.0', '>= 1.0.2' gem 'pg' +gem 'rails', '~> 4.2.4' gem 'rails_admin', '~> 1.0' gem 'validates_formatting_of', '~> 0.9.0' -gem 'tzinfo-data', platforms: [:mingw, :mswin, :x64_mingw] gem 'paranoia', '~> 2.2' +gem 'tzinfo-data', platforms: %i[mingw mswin x64_mingw] + +gem 'byebug', groups: %i[development test] +gem 'dotenv-rails', groups: %i[development test] group :assets do gem 'sass-rails', '>= 4.0.3' @@ -23,19 +25,18 @@ group :assets do end group :development do - gem 'byebug' gem 'spring' end group :production do - gem 'rails_12factor' gem 'puma' + gem 'rails_12factor' gem 'skylight' end group :test do gem 'coveralls', require: false - gem 'simplecov', require: false gem 'rubocop' + gem 'simplecov', require: false gem 'webmock' end diff --git a/Rakefile b/Rakefile index 7172d2e7..ef2a2174 100644 --- a/Rakefile +++ b/Rakefile @@ -12,4 +12,4 @@ end Rails.application.load_tasks -task default: [:rubocop, :test] +task default: %i[rubocop test] diff --git a/app/controllers/info_window_controller.rb b/app/controllers/info_window_controller.rb index 8e1508f4..853da2a7 100644 --- a/app/controllers/info_window_controller.rb +++ b/app/controllers/info_window_controller.rb @@ -1,6 +1,6 @@ class InfoWindowController < ApplicationController def index - @thing = Thing.find_by_id(params[:thing_id]) + @thing = Thing.find_by(id: params[:thing_id]) view = begin if @thing.adopted? user_signed_in? && current_user == @thing.user ? 'users/thank_you' : 'users/profile' diff --git a/app/controllers/reminders_controller.rb b/app/controllers/reminders_controller.rb index b1eb8eb2..65b9e764 100644 --- a/app/controllers/reminders_controller.rb +++ b/app/controllers/reminders_controller.rb @@ -8,7 +8,7 @@ def create @reminder.from_user = current_user if @reminder.save ThingMailer.reminder(@reminder.thing).deliver_later - @reminder.update_attribute(:sent, true) + @reminder.update_attributes(sent: true) render(json: @reminder) else render(json: {errors: @reminder.errors}, status: 500) diff --git a/config/initializers/airbrake.rb b/config/initializers/airbrake.rb index 86b222f2..f20b01d0 100644 --- a/config/initializers/airbrake.rb +++ b/config/initializers/airbrake.rb @@ -41,7 +41,7 @@ # environments. # NOTE: This option *does not* work if you don't set the 'environment' option. # https://github.com/airbrake/airbrake-ruby#ignore_environments - c.ignore_environments = %w(development test) + c.ignore_environments = %w[development test] # A list of parameters that should be filtered out of what is sent to # Airbrake. By default, all "password" attributes will have their contents diff --git a/db/migrate/00000000000005_create_rails_admin_histories_table.rb b/db/migrate/00000000000005_create_rails_admin_histories_table.rb index 97ddaecc..77661f3a 100644 --- a/db/migrate/00000000000005_create_rails_admin_histories_table.rb +++ b/db/migrate/00000000000005_create_rails_admin_histories_table.rb @@ -10,6 +10,6 @@ def change t.timestamps end - add_index(:rails_admin_histories, [:item, :table, :month, :year], name: 'index_rails_admin_histories') + add_index(:rails_admin_histories, %i[item table month year], name: 'index_rails_admin_histories') end end diff --git a/lib/thing_importer.rb b/lib/thing_importer.rb index f83e5366..193ab08a 100644 --- a/lib/thing_importer.rb +++ b/lib/thing_importer.rb @@ -48,15 +48,15 @@ def import_temp_things(source_url) insert_statement_id = SecureRandom.uuid conn = ActiveRecord::Base.connection - conn.execute(<<-SQL) -CREATE TEMPORARY TABLE "temp_thing_import" ( - id serial, - name varchar, - lat numeric(16,14), - lng numeric(17,14), - city_id integer, - system_use_code varchar -) + conn.execute(<<-SQL.strip_heredoc) + CREATE TEMPORARY TABLE "temp_thing_import" ( + id serial, + name varchar, + lat numeric(16,14), + lng numeric(17,14), + city_id integer, + system_use_code varchar + ) SQL conn.raw_connection.prepare(insert_statement_id, 'INSERT INTO temp_thing_import (name, lat, lng, city_id, system_use_code) VALUES($1, $2, $3, $4, $5)') @@ -67,7 +67,8 @@ def import_temp_things(source_url) each do |thing| conn.raw_connection.exec_prepared( insert_statement_id, - [thing[:type], thing[:lat], thing[:lng], thing[:city_id], thing[:system_use_code]]) + [thing[:type], thing[:lat], thing[:lng], thing[:city_id], thing[:system_use_code]], + ) end conn.execute('CREATE INDEX "temp_thing_import_city_id" ON temp_thing_import(city_id)') @@ -77,11 +78,11 @@ def import_temp_things(source_url) # return the deleted drains partitioned by whether they were adopted def delete_non_existing_things # mark deleted_at as this is what the paranoia gem uses to scope - deleted_things = ActiveRecord::Base.connection.execute(<<-SQL) -UPDATE things -SET deleted_at = NOW() -WHERE things.city_id NOT IN (SELECT city_id from temp_thing_import) AND deleted_at IS NULL -RETURNING things.city_id, things.user_id + deleted_things = ActiveRecord::Base.connection.execute(<<-SQL.strip_heredoc) + UPDATE things + SET deleted_at = NOW() + WHERE things.city_id NOT IN (SELECT city_id from temp_thing_import) AND deleted_at IS NULL + RETURNING things.city_id, things.user_id SQL deleted_things.partition { |thing| thing['user_id'].present? } end @@ -89,24 +90,24 @@ def delete_non_existing_things def upsert_things # postgresql's RETURNING returns both updated and inserted records so we # query for the items to be inserted first - created_things = ActiveRecord::Base.connection.execute(<<-SQL) -SELECT temp_thing_import.city_id -FROM things -RIGHT JOIN temp_thing_import ON temp_thing_import.city_id = things.city_id -WHERE things.id IS NULL + created_things = ActiveRecord::Base.connection.execute(<<-SQL.strip_heredoc) + SELECT temp_thing_import.city_id + FROM things + RIGHT JOIN temp_thing_import ON temp_thing_import.city_id = things.city_id + WHERE things.id IS NULL SQL - ActiveRecord::Base.connection.execute(<<-SQL) -INSERT INTO things(name, lat, lng, city_id, system_use_code) -SELECT name, lat, lng, city_id, system_use_code FROM temp_thing_import -ON CONFLICT(city_id) DO UPDATE SET - lat = EXCLUDED.lat, - lng = EXCLUDED.lng, - name = CASE - WHEN things.user_id IS NOT NULL THEN things.name - ELSE EXCLUDED.name - END, - deleted_at = NULL + ActiveRecord::Base.connection.execute(<<-SQL.strip_heredoc) + INSERT INTO things(name, lat, lng, city_id, system_use_code) + SELECT name, lat, lng, city_id, system_use_code FROM temp_thing_import + ON CONFLICT(city_id) DO UPDATE SET + lat = EXCLUDED.lat, + lng = EXCLUDED.lng, + name = CASE + WHEN things.user_id IS NOT NULL THEN things.name + ELSE EXCLUDED.name + END, + deleted_at = NULL SQL created_things diff --git a/test/controllers/info_window_controller_test.rb b/test/controllers/info_window_controller_test.rb index 038d7865..2e0b739f 100644 --- a/test/controllers/info_window_controller_test.rb +++ b/test/controllers/info_window_controller_test.rb @@ -64,7 +64,7 @@ class InfoWindowControllerTest < ActionController::TestCase test 'should show special link on adoption form if it has one' do sign_in @user - Thing.stub :find_by_id, @thing do + Thing.stub :find_by, @thing do @thing.stub :detail_link, 'http://example.com' do get :index, thing_id: @thing.id end diff --git a/test/lib/thing_importer_test.rb b/test/lib/thing_importer_test.rb index 45086ba7..afeaae42 100644 --- a/test/lib/thing_importer_test.rb +++ b/test/lib/thing_importer_test.rb @@ -4,21 +4,21 @@ class ThingImporterTest < ActiveSupport::TestCase test 'import does not modify data if endpoint fails' do - thing_1 = things(:thing_1) + thing1 = things(:thing_1) fake_url = 'http://sf-drain-data.org' stub_request(:get, fake_url).to_return(status: [500, 'Internal Server Error'], body: nil) assert_raises OpenURI::HTTPError do ThingImporter.load(fake_url) end - assert_not_nil Thing.find(thing_1.id) + assert_not_nil Thing.find(thing1.id) end test 'loading things, deletes existing things not in data set, updates properties on rest' do admin = users(:admin) - thing_1 = things(:thing_1) - thing_11 = things(:thing_11) - thing_10 = things(:thing_10).tap do |thing| + thing1 = things(:thing_1) + thing11 = things(:thing_11) + thing10 = things(:thing_10).tap do |thing| thing.update!(name: 'Erik drain', user_id: users(:erik).id) end things(:thing_9).tap do |thing| @@ -43,11 +43,11 @@ class ThingImporterTest < ActiveSupport::TestCase email = ActionMailer::Base.deliveries.last assert_equal email.to, [admin.email] assert_equal email.subject, 'Adopt-a-Drain San Francisco import (1 adopted drains removed, 1 drains added, 7 unadopted drains removed)' - thing_11.reload - thing_10.reload + thing11.reload + thing10.reload # Asserts thing_1 is deleted - assert_nil Thing.find_by(id: thing_1.id) + assert_nil Thing.find_by(id: thing1.id) # Asserts thing_3 is reified assert_equal Thing.find_by(city_id: 3).id, deleted_thing.id @@ -59,12 +59,12 @@ class ThingImporterTest < ActiveSupport::TestCase assert_equal new_thing.lng, BigDecimal.new(-121.40, 16) # Asserts properties on thing_11 have been updated - assert_equal thing_11.lat, BigDecimal.new(37.75, 16) - assert_equal thing_11.lng, BigDecimal.new(-122.40, 16) + assert_equal thing11.lat, BigDecimal.new(37.75, 16) + assert_equal thing11.lng, BigDecimal.new(-122.40, 16) # Asserts properties on thing_10 have been updated, but not the name - assert_equal 'Erik drain', thing_10.name - assert_equal BigDecimal.new(36.75, 16), thing_10.lat - assert_equal BigDecimal.new(-121.40, 16), thing_10.lng + assert_equal 'Erik drain', thing10.name + assert_equal BigDecimal.new(36.75, 16), thing10.lat + assert_equal BigDecimal.new(-121.40, 16), thing10.lng end end diff --git a/test/mailers/thing_mailer_test.rb b/test/mailers/thing_mailer_test.rb index 0bf95c95..d5a7e1a9 100644 --- a/test/mailers/thing_mailer_test.rb +++ b/test/mailers/thing_mailer_test.rb @@ -41,9 +41,9 @@ class ThingMailerTest < ActionMailer::TestCase end test 'thing_update_report' do - admin_1 = users(:admin) - admin_2 = users(:admin) - admin_2.update(email: 'admin2@example.com') + admin1 = users(:admin) + admin2 = users(:admin) + admin2.update(email: 'admin2@example.com') email = nil deleted_thing = things(:thing_1) @@ -51,8 +51,8 @@ class ThingMailerTest < ActionMailer::TestCase email = ThingMailer.thing_update_report([deleted_thing], [], []).deliver_now end - assert_includes email.to, admin_1.email - assert_includes email.to, admin_2.email + assert_includes email.to, admin1.email + assert_includes email.to, admin2.email assert_equal email.subject, 'Adopt-a-Drain San Francisco import (1 adopted drains removed, 0 drains added, 0 unadopted drains removed)' end