From: Tom Hughes Date: Thu, 6 Aug 2020 20:56:18 +0000 (+0100) Subject: Fix some new rubocop warnings X-Git-Tag: live~2511 X-Git-Url: https://git.openstreetmap.org./rails.git/commitdiff_plain/ea59d95f4aad7cafe3a0d0b4d5ef533b0662e365?ds=sidebyside Fix some new rubocop warnings --- diff --git a/.rubocop_todo.yml b/.rubocop_todo.yml index 1baaf0ab6..32a067c19 100644 --- a/.rubocop_todo.yml +++ b/.rubocop_todo.yml @@ -1,10 +1,11 @@ # This configuration was generated by # `rubocop --auto-gen-config` -# on 2020-08-06 17:20:38 UTC using RuboCop version 0.89.0. +# on 2020-08-06 20:53:30 UTC using RuboCop version 0.89.0. # The point is for the user to remove these configuration records # one by one as the offenses are removed from the code base. # Note that changes in the inspected code, or installation of new # versions of RuboCop, may require this file to be generated again. +# versions of RuboCop, may require this file to be generated again. # Work around erblint issues. # https://github.com/openstreetmap/openstreetmap-website/issues/2472 @@ -87,6 +88,25 @@ Metrics/ParameterLists: Metrics/PerceivedComplexity: Max: 26 +# Offense count: 540 +Minitest/MultipleAssertions: + Max: 81 + +# Offense count: 26 +# Cop supports --auto-correct. +Minitest/TestMethodName: + Exclude: + - 'test/abilities/api_capability_test.rb' + - 'test/controllers/api/nodes_controller_test.rb' + - 'test/controllers/api/old_nodes_controller_test.rb' + - 'test/controllers/api/relations_controller_test.rb' + - 'test/controllers/api/ways_controller_test.rb' + - 'test/helpers/browse_helper_test.rb' + - 'test/integration/client_applications_test.rb' + - 'test/integration/short_links_test.rb' + - 'test/integration/user_blocks_test.rb' + - 'test/integration/user_creation_test.rb' + # Offense count: 6 Naming/AccessorMethodName: Exclude: @@ -137,6 +157,14 @@ Rails/HelperInstanceVariable: Exclude: - 'app/helpers/title_helper.rb' +# Offense count: 1 +# Cop supports --auto-correct. +# Configuration parameters: Include. +# Include: app/mailers/**/*.rb +Rails/MailerName: + Exclude: + - 'app/mailers/notifier.rb' + # Offense count: 5 # Configuration parameters: Include. # Include: db/migrate/*.rb diff --git a/app/controllers/user_blocks_controller.rb b/app/controllers/user_blocks_controller.rb index 70dfddf83..5e0a7ee70 100644 --- a/app/controllers/user_blocks_controller.rb +++ b/app/controllers/user_blocks_controller.rb @@ -128,7 +128,7 @@ class UserBlocksController < ApplicationController @block_period = params[:user_block_period].to_i @valid_params = false - if !UserBlock::PERIODS.include?(@block_period) + if UserBlock::PERIODS.exclude?(@block_period) flash[:error] = t("user_blocks.filter.block_period") elsif @user_block && !@user_block.active? diff --git a/app/models/acl.rb b/app/models/acl.rb index 06f754c22..90dd7f3cf 100644 --- a/app/models/acl.rb +++ b/app/models/acl.rb @@ -30,14 +30,14 @@ class Acl < ApplicationRecord end def self.no_account_creation(address, options = {}) - match(address, options).where(:k => "no_account_creation").exists? + match(address, options).exists?(:k => "no_account_creation") end def self.no_note_comment(address, domain = nil) - match(address, :domain => domain).where(:k => "no_note_comment").exists? + match(address, :domain => domain).exists?(:k => "no_note_comment") end def self.no_trace_download(address, domain = nil) - match(address, :domain => domain).where(:k => "no_trace_download").exists? + match(address, :domain => domain).exists?(:k => "no_trace_download") end end diff --git a/app/models/user.rb b/app/models/user.rb index 518cb94cc..92fb3287b 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -218,7 +218,7 @@ class User < ApplicationRecord end def is_friends_with?(new_friend) - friendships.where(:befriendee => new_friend).exists? + friendships.exists?(:befriendee => new_friend) end ## diff --git a/lib/bounding_box.rb b/lib/bounding_box.rb index 630518649..0b9731b22 100644 --- a/lib/bounding_box.rb +++ b/lib/bounding_box.rb @@ -89,7 +89,7 @@ class BoundingBox end def complete? - !to_a.include?(nil) + to_a.exclude?(nil) end def centre_lon diff --git a/lib/classic_pagination/pagination.rb b/lib/classic_pagination/pagination.rb index 161cc7fc7..53e730afc 100644 --- a/lib/classic_pagination/pagination.rb +++ b/lib/classic_pagination/pagination.rb @@ -158,7 +158,7 @@ module ActionController def create_paginators_and_retrieve_collections #:nodoc: Pagination::OPTIONS[self.class].each do |collection_id, options| - next if options[:actions] && !options[:actions].include?(action_name) + next if options[:actions]&.exclude?(action_name) paginator, collection = paginator_and_collection_for(collection_id, options) diff --git a/lib/locale.rb b/lib/locale.rb index 56d398576..5931301b5 100644 --- a/lib/locale.rb +++ b/lib/locale.rb @@ -17,7 +17,7 @@ class Locale < I18n::Locale::Tag::Rfc4646 def expand List.new(reverse.each_with_object([]) do |locale, expanded| locale.candidates.uniq.reverse_each do |candidate| - expanded << candidate if candidate == locale || !expanded.include?(candidate) + expanded << candidate if candidate == locale || expanded.exclude?(candidate) end end.reverse.uniq) end diff --git a/script/update-spam-blocks b/script/update-spam-blocks index f14905b90..86e853cfc 100755 --- a/script/update-spam-blocks +++ b/script/update-spam-blocks @@ -13,7 +13,7 @@ addresses = User.count( addresses.each do |address, count| next unless count > 1 - next if Acl.where(:address => address).exists? + next if Acl.exists?(:address => address) Acl.create({ :address => address, diff --git a/test/controllers/api/amf_controller_test.rb b/test/controllers/api/amf_controller_test.rb index c62c750b6..47cced6ff 100644 --- a/test/controllers/api/amf_controller_test.rb +++ b/test/controllers/api/amf_controller_test.rb @@ -291,7 +291,7 @@ module Api assert_response :success amf_parse_response rel = amf_result("/1") - assert_equal rel[0], 0 + assert_equal(0, rel[0]) assert_equal rel[2], id end @@ -301,8 +301,8 @@ module Api assert_response :success amf_parse_response rel = amf_result("/1") - assert_equal rel[0], -4 - assert_equal rel[1], "relation" + assert_equal(-4, rel[0]) + assert_equal("relation", rel[1]) assert_equal rel[2], id assert(rel[3].nil?) && rel[4].nil? end @@ -313,8 +313,8 @@ module Api assert_response :success amf_parse_response rel = amf_result("/1") - assert_equal rel[0], -4 - assert_equal rel[1], "relation" + assert_equal(-4, rel[0]) + assert_equal("relation", rel[1]) assert_equal rel[2], id assert(rel[3].nil?) && rel[4].nil? end @@ -426,8 +426,8 @@ module Api history = amf_result("/1") # ['way',wayid,history] - assert_equal history[0], "way" - assert_equal history[1], 0 + assert_equal("way", history[0]) + assert_equal(0, history[1]) assert_empty history[2] end @@ -445,8 +445,7 @@ module Api # ['node',nodeid,history] # note that (as per getway_history) we actually round up # to the next second - assert_equal history[0], "node", - 'first element should be "node"' + assert_equal("node", history[0], 'first element should be "node"') assert_equal history[1], node.id, "second element should be the input node ID" assert_equal history[2].first[0], @@ -464,8 +463,8 @@ module Api history = amf_result("/1") # ['node',nodeid,history] - assert_equal history[0], "node" - assert_equal history[1], 0 + assert_equal("node", history[0]) + assert_equal(0, history[1]) assert_empty history[2] end @@ -979,15 +978,15 @@ module Api new_node = Node.find(new_node_id) assert_equal 1, new_node.version assert new_node.visible - assert_equal 4.56, new_node.lon - assert_equal 12.34, new_node.lat + assert_in_delta(4.56, new_node.lon) + assert_in_delta(12.34, new_node.lat) assert_equal({ "test" => "new" }, new_node.tags) changed_node = Node.find(d) assert_equal 2, changed_node.version assert changed_node.visible - assert_equal 12.34, changed_node.lon - assert_equal 4.56, changed_node.lat + assert_in_delta(12.34, changed_node.lon) + assert_in_delta(4.56, changed_node.lat) assert_equal({ "test" => "ok" }, changed_node.tags) # node is not deleted because our other ways are using it @@ -1074,15 +1073,15 @@ module Api new_node = Node.find(new_node_id) assert_equal 1, new_node.version assert new_node.visible - assert_equal 4.56, new_node.lon - assert_equal 12.34, new_node.lat + assert_in_delta(4.56, new_node.lon) + assert_in_delta(12.34, new_node.lat) assert_equal({ "test" => "new" }, new_node.tags) changed_node = Node.find(b) assert_equal 2, changed_node.version assert changed_node.visible - assert_equal 12.34, changed_node.lon - assert_equal 4.56, changed_node.lat + assert_in_delta(12.34, changed_node.lon) + assert_in_delta(4.56, changed_node.lat) assert_equal({ "test" => "ok" }, changed_node.tags) deleted_node = Node.find(d) diff --git a/test/controllers/api/changes_controller_test.rb b/test/controllers/api/changes_controller_test.rb index 79b112fb1..88d3c2e0e 100644 --- a/test/controllers/api/changes_controller_test.rb +++ b/test/controllers/api/changes_controller_test.rb @@ -60,7 +60,7 @@ module Api zoom_to_test.each do |zoom| get changes_path(:zoom => zoom) assert_response :bad_request - assert_equal @response.body, "Requested zoom is invalid, or the supplied start is after the end time, or the start duration is more than 24 hours" + assert_equal("Requested zoom is invalid, or the supplied start is after the end time, or the start duration is more than 24 hours", @response.body) end end @@ -81,7 +81,7 @@ module Api invalid.each do |hour| get changes_path(:hours => hour) assert_response :bad_request, "Problem with the hour: #{hour}" - assert_equal @response.body, "Requested zoom is invalid, or the supplied start is after the end time, or the start duration is more than 24 hours", "Problem with the hour: #{hour}." + assert_equal("Requested zoom is invalid, or the supplied start is after the end time, or the start duration is more than 24 hours", @response.body, "Problem with the hour: #{hour}.") end end @@ -95,7 +95,7 @@ module Api def test_changes_start_end_invalid get changes_path(:start => "2010-04-03 10:55:00", :end => "2010-04-03 09:55:00") assert_response :bad_request - assert_equal @response.body, "Requested zoom is invalid, or the supplied start is after the end time, or the start duration is more than 24 hours" + assert_equal("Requested zoom is invalid, or the supplied start is after the end time, or the start duration is more than 24 hours", @response.body) end def test_changes_start_end_valid diff --git a/test/controllers/api/changesets_controller_test.rb b/test/controllers/api/changesets_controller_test.rb index 3fecebb23..871eac9ed 100644 --- a/test/controllers/api/changesets_controller_test.rb +++ b/test/controllers/api/changesets_controller_test.rb @@ -908,7 +908,7 @@ module Api CHANGESET post changeset_upload_path(changeset), :params => diff, :headers => auth_header assert_response :bad_request, "Shouldn't be able to upload a diff with the action ping" - assert_equal @response.body, "Unknown action ping, choices are create, modify, delete" + assert_equal("Unknown action ping, choices are create, modify, delete", @response.body) end ## diff --git a/test/controllers/api/map_controller_test.rb b/test/controllers/api/map_controller_test.rb index dd7bb2cb3..607c4e421 100644 --- a/test/controllers/api/map_controller_test.rb +++ b/test/controllers/api/map_controller_test.rb @@ -181,7 +181,7 @@ module Api .select { |a| a["version"] == node.version } .select { |a| a["changeset"] == node.changeset_id } .select { |a| a["timestamp"] == node.timestamp.xmlschema } - assert_equal result_nodes.count, 1 + assert_equal(1, result_nodes.count) result_node = result_nodes.first assert_equal result_node["tags"], tag.k => tag.v diff --git a/test/controllers/api/relations_controller_test.rb b/test/controllers/api/relations_controller_test.rb index b04ad3c19..559f19bd6 100644 --- a/test/controllers/api/relations_controller_test.rb +++ b/test/controllers/api/relations_controller_test.rb @@ -296,10 +296,8 @@ module Api assert_not_nil checkrelation, "uploaded relation not found in data base after upload" # compare values - assert_equal checkrelation.members.length, 0, - "saved relation contains members but should not" - assert_equal checkrelation.tags.length, 1, - "saved relation does not contain exactly one tag" + assert_equal(0, checkrelation.members.length, "saved relation contains members but should not") + assert_equal(1, checkrelation.tags.length, "saved relation does not contain exactly one tag") assert_equal changeset.id, checkrelation.changeset.id, "saved relation does not belong in the changeset it was assigned to" assert_equal user.id, checkrelation.changeset.user_id, @@ -326,10 +324,8 @@ module Api assert_not_nil checkrelation, "uploaded relation not found in data base after upload" # compare values - assert_equal checkrelation.members.length, 1, - "saved relation does not contain exactly one member" - assert_equal checkrelation.tags.length, 1, - "saved relation does not contain exactly one tag" + assert_equal(1, checkrelation.members.length, "saved relation does not contain exactly one member") + assert_equal(1, checkrelation.tags.length, "saved relation does not contain exactly one tag") assert_equal changeset.id, checkrelation.changeset.id, "saved relation does not belong in the changeset it was assigned to" assert_equal user.id, checkrelation.changeset.user_id, @@ -356,10 +352,8 @@ module Api assert_not_nil checkrelation, "uploaded relation not found in data base after upload" # compare values - assert_equal checkrelation.members.length, 1, - "saved relation does not contain exactly one member" - assert_equal checkrelation.tags.length, 1, - "saved relation does not contain exactly one tag" + assert_equal(1, checkrelation.members.length, "saved relation does not contain exactly one member") + assert_equal(1, checkrelation.tags.length, "saved relation does not contain exactly one tag") assert_equal changeset.id, checkrelation.changeset.id, "saved relation does not belong in the changeset it was assigned to" assert_equal user.id, checkrelation.changeset.user_id, @@ -387,10 +381,8 @@ module Api assert_not_nil checkrelation, "uploaded relation not found in data base after upload" # compare values - assert_equal checkrelation.members.length, 2, - "saved relation does not have exactly two members" - assert_equal checkrelation.tags.length, 1, - "saved relation does not contain exactly one tag" + assert_equal(2, checkrelation.members.length, "saved relation does not have exactly two members") + assert_equal(1, checkrelation.tags.length, "saved relation does not contain exactly one tag") assert_equal changeset.id, checkrelation.changeset.id, "saved relation does not belong in the changeset it was assigned to" assert_equal user.id, checkrelation.changeset.user_id, diff --git a/test/controllers/api/traces_controller_test.rb b/test/controllers/api/traces_controller_test.rb index ea8476453..c2ddb6031 100644 --- a/test/controllers/api/traces_controller_test.rb +++ b/test/controllers/api/traces_controller_test.rb @@ -307,7 +307,7 @@ module Api updated = Trace.find(trace.id) # Ensure there's only one tag in the database after updating - assert_equal Tracetag.count, 1 + assert_equal(1, Tracetag.count) # The new tag object might have a different id, so check the string representation assert_equal trace.tagstring, updated.tagstring end diff --git a/test/controllers/api/ways_controller_test.rb b/test/controllers/api/ways_controller_test.rb index 672a282a9..74ef9f524 100644 --- a/test/controllers/api/ways_controller_test.rb +++ b/test/controllers/api/ways_controller_test.rb @@ -180,8 +180,7 @@ module Api assert_not_nil checkway, "uploaded way not found in data base after upload" # compare values - assert_equal checkway.nds.length, 2, - "saved way does not contain exactly one node" + assert_equal(2, checkway.nds.length, "saved way does not contain exactly one node") assert_equal checkway.nds[0], node1.id, "saved way does not contain the right node on pos 0" assert_equal checkway.nds[1], node2.id, diff --git a/test/controllers/site_controller_test.rb b/test/controllers/site_controller_test.rb index 77b07db7d..b7571b1f5 100644 --- a/test/controllers/site_controller_test.rb +++ b/test/controllers/site_controller_test.rb @@ -275,8 +275,8 @@ class SiteControllerTest < ActionDispatch::IntegrationTest assert_response :success assert_template "edit" - assert_equal 1.0, assigns(:lat) - assert_equal 1.0, assigns(:lon) + assert_in_delta(1.0, assigns(:lat)) + assert_in_delta(1.0, assigns(:lon)) assert_equal 18, assigns(:zoom) end @@ -312,8 +312,8 @@ class SiteControllerTest < ActionDispatch::IntegrationTest get edit_path(:way => way.id) assert_response :success assert_template "edit" - assert_equal 3.0, assigns(:lat) - assert_equal 3.0, assigns(:lon) + assert_in_delta(3.0, assigns(:lat)) + assert_in_delta(3.0, assigns(:lon)) assert_equal 17, assigns(:zoom) end @@ -349,8 +349,8 @@ class SiteControllerTest < ActionDispatch::IntegrationTest get edit_path(:note => note.id) assert_response :success assert_template "edit" - assert_equal 1.0, assigns(:lat) - assert_equal 1.0, assigns(:lon) + assert_in_delta(1.0, assigns(:lat)) + assert_in_delta(1.0, assigns(:lon)) assert_equal 17, assigns(:zoom) end @@ -386,8 +386,8 @@ class SiteControllerTest < ActionDispatch::IntegrationTest get edit_path(:gpx => gpx.id) assert_response :success assert_template "edit" - assert_equal 1.0, assigns(:lat) - assert_equal 1.0, assigns(:lon) + assert_in_delta(1.0, assigns(:lat)) + assert_in_delta(1.0, assigns(:lon)) assert_equal 16, assigns(:zoom) end diff --git a/test/helpers/user_helper_test.rb b/test/helpers/user_helper_test.rb index 397a90a63..c2b54c056 100644 --- a/test/helpers/user_helper_test.rb +++ b/test/helpers/user_helper_test.rb @@ -73,10 +73,10 @@ class UserHelperTest < ActionView::TestCase def test_auth_button button = auth_button("google", "google") - assert_equal button, "\"Login" + assert_equal("\"Login", button) button = auth_button("yahoo", "openid", :openid_url => "yahoo.com") - assert_equal button, "\"Login" + assert_equal("\"Login", button) end private diff --git a/test/lib/country_test.rb b/test/lib/country_test.rb index a6fc7c144..596690c6c 100644 --- a/test/lib/country_test.rb +++ b/test/lib/country_test.rb @@ -5,20 +5,20 @@ class CountryTest < ActiveSupport::TestCase gb = Country.find("GB") assert_not_nil gb assert_equal "GB", gb.code - assert_equal(-8.623555, gb.min_lon) - assert_equal 59.360249, gb.max_lat - assert_equal 1.759, gb.max_lon - assert_equal 49.906193, gb.min_lat + assert_in_delta(-8.623555, gb.min_lon) + assert_in_delta(59.360249, gb.max_lat) + assert_in_delta(1.759, gb.max_lon) + assert_in_delta(49.906193, gb.min_lat) end def test_au au = Country.find("AU") assert_not_nil au assert_equal "AU", au.code - assert_equal 112.911057, au.min_lon - assert_equal(-10.062805, au.max_lat) - assert_equal 153.639252, au.max_lon - assert_equal(-43.64397, au.min_lat) + assert_in_delta(112.911057, au.min_lon) + assert_in_delta(-10.062805, au.max_lat) + assert_in_delta(153.639252, au.max_lon) + assert_in_delta(-43.64397, au.min_lat) end def test_xx diff --git a/test/models/message_test.rb b/test/models/message_test.rb index 73ae5f76c..b1acd0805 100644 --- a/test/models/message_test.rb +++ b/test/models/message_test.rb @@ -64,7 +64,7 @@ class MessageTest < ActiveSupport::TestCase db_msg = msg.class.find(msg.id) assert_equal char, db_msg.title, "Database silently truncated message title" rescue ArgumentError => e - assert_equal e.to_s, "invalid byte sequence in UTF-8" + assert_equal("invalid byte sequence in UTF-8", e.to_s) end end diff --git a/test/models/node_test.rb b/test/models/node_test.rb index 5733d5063..b5435412b 100644 --- a/test/models/node_test.rb +++ b/test/models/node_test.rb @@ -88,7 +88,7 @@ class NodeTest < ActiveSupport::TestCase assert_equal node_template.visible, node.visible assert_equal node_template.timestamp.to_i, node.timestamp.to_i - assert_equal OldNode.where(:node_id => node_template.id).count, 1 + assert_equal(1, OldNode.where(:node_id => node_template.id).count) old_node = OldNode.where(:node_id => node_template.id).first assert_not_nil old_node assert_equal node_template.latitude, old_node.latitude @@ -105,7 +105,7 @@ class NodeTest < ActiveSupport::TestCase node_template = Node.find(node.id) assert_not_nil node_template - assert_equal OldNode.where(:node_id => node_template.id).count, 1 + assert_equal(1, OldNode.where(:node_id => node_template.id).count) assert_not_nil node node_template.lat = 12.3456 @@ -121,7 +121,7 @@ class NodeTest < ActiveSupport::TestCase assert_equal node_template.visible, node.visible # assert_equal node_template.tags, node.tags - assert_equal OldNode.where(:node_id => node_template.id).count, 2 + assert_equal(2, OldNode.where(:node_id => node_template.id).count) old_node = OldNode.where(:node_id => node_template.id, :version => 2).first assert_not_nil old_node assert_equal node_template.latitude, old_node.latitude @@ -137,7 +137,7 @@ class NodeTest < ActiveSupport::TestCase node_template = Node.find(node.id) assert_not_nil node_template - assert_equal OldNode.where(:node_id => node_template.id).count, 1 + assert_equal(1, OldNode.where(:node_id => node_template.id).count) assert_not_nil node assert node.delete_with_history!(node_template, node.changeset.user) @@ -150,7 +150,7 @@ class NodeTest < ActiveSupport::TestCase assert_not node.visible # assert_equal node_template.tags, node.tags - assert_equal OldNode.where(:node_id => node_template.id).count, 2 + assert_equal(2, OldNode.where(:node_id => node_template.id).count) old_node = OldNode.where(:node_id => node_template.id, :version => 2).first assert_not_nil old_node assert_equal node_template.latitude, old_node.latitude diff --git a/test/models/trace_test.rb b/test/models/trace_test.rb index bf4e0196b..8b38b7f0e 100644 --- a/test/models/trace_test.rb +++ b/test/models/trace_test.rb @@ -222,7 +222,7 @@ class TraceTest < ActiveSupport::TestCase trace.import - assert File.exist?(icon_path) + assert_path_exists(icon_path) end def test_import_creates_large_picture @@ -233,7 +233,7 @@ class TraceTest < ActiveSupport::TestCase trace.import - assert File.exist?(large_picture_path) + assert_path_exists(large_picture_path) end def test_import_handles_bz2 diff --git a/test/system/issues_test.rb b/test/system/issues_test.rb index ebf1d272a..1fdbe1373 100644 --- a/test/system/issues_test.rb +++ b/test/system/issues_test.rb @@ -83,7 +83,7 @@ class IssuesTest < ApplicationSystemTestCase assert page.has_content?("test comment") issue.reload - assert_equal issue.comments.first.body, "test comment" + assert_equal("test comment", issue.comments.first.body) end def test_reassign_issue diff --git a/test/test_helper.rb b/test/test_helper.rb index 40b2b50bc..720618ba0 100644 --- a/test/test_helper.rb +++ b/test/test_helper.rb @@ -175,7 +175,7 @@ module ActiveSupport # when the owner of the changset has their data not marked as public def assert_require_public_data(msg = "Shouldn't be able to use API when the user's data is not public") assert_response :forbidden, msg - assert_equal @response.headers["Error"], "You must make your edits public to upload new data", "Wrong error message" + assert_equal("You must make your edits public to upload new data", @response.headers["Error"], "Wrong error message") end ##