From 1f242978af5400bbe53d61491d0f7e8ba563d9ae Mon Sep 17 00:00:00 2001 From: Andy Allan Date: Wed, 11 Nov 2020 15:51:44 +0000 Subject: [PATCH 1/2] Prefer keyword arguments when method has optional boolean arguments --- .rubocop_todo.yml | 3 --- test/helpers/browse_helper_test.rb | 16 ++++++++-------- test/models/diary_entry_test.rb | 18 +++++++++--------- test/models/trace_test.rb | 14 +++++++------- test/models/tracetag_test.rb | 20 ++++++++++---------- 5 files changed, 34 insertions(+), 37 deletions(-) diff --git a/.rubocop_todo.yml b/.rubocop_todo.yml index cfff8c43e2..f6d11c58f9 100644 --- a/.rubocop_todo.yml +++ b/.rubocop_todo.yml @@ -185,9 +185,6 @@ Style/OptionalBooleanParameter: - 'app/models/trace.rb' - 'app/models/tracepoint.rb' - 'app/models/way.rb' - - 'test/models/diary_entry_test.rb' - - 'test/models/trace_test.rb' - - 'test/models/tracetag_test.rb' # Offense count: 28 # Cop supports --auto-correct. diff --git a/test/helpers/browse_helper_test.rb b/test/helpers/browse_helper_test.rb index bc597596f5..076975b098 100644 --- a/test/helpers/browse_helper_test.rb +++ b/test/helpers/browse_helper_test.rb @@ -31,8 +31,8 @@ def test_printable_name assert_dom_equal "Test Node (#{node.id})", printable_name(node) assert_dom_equal "Test Node (#{node.id})", printable_name(node_v2) assert_dom_equal node.id.to_s, printable_name(node_v1) - assert_dom_equal "Test Node (#{node.id}, v2)", printable_name(node_v2, true) - assert_dom_equal "#{node.id}, v1", printable_name(node_v1, true) + assert_dom_equal "Test Node (#{node.id}, v2)", printable_name(node_v2, :version => true) + assert_dom_equal "#{node.id}, v1", printable_name(node_v1, :version => true) assert_dom_equal "3.1415926 (#{node_with_ref_without_name.id})", printable_name(node_with_ref_without_name) I18n.locale = "pt" @@ -41,8 +41,8 @@ def test_printable_name assert_dom_equal "Nó teste (#{node.id})", printable_name(node) assert_dom_equal "Nó teste (#{node.id})", printable_name(node_v2) assert_dom_equal node.id.to_s, printable_name(node_v1) - assert_dom_equal "Nó teste (#{node.id}, v2)", printable_name(node_v2, true) - assert_dom_equal "#{node.id}, v1", printable_name(node_v1, true) + assert_dom_equal "Nó teste (#{node.id}, v2)", printable_name(node_v2, :version => true) + assert_dom_equal "#{node.id}, v1", printable_name(node_v1, :version => true) assert_dom_equal "3.1415926 (#{node_with_ref_without_name.id})", printable_name(node_with_ref_without_name) I18n.locale = "pt-BR" @@ -51,8 +51,8 @@ def test_printable_name assert_dom_equal "Nó teste (#{node.id})", printable_name(node) assert_dom_equal "Nó teste (#{node.id})", printable_name(node_v2) assert_dom_equal node.id.to_s, printable_name(node_v1) - assert_dom_equal "Nó teste (#{node.id}, v2)", printable_name(node_v2, true) - assert_dom_equal "#{node.id}, v1", printable_name(node_v1, true) + assert_dom_equal "Nó teste (#{node.id}, v2)", printable_name(node_v2, :version => true) + assert_dom_equal "#{node.id}, v1", printable_name(node_v1, :version => true) assert_dom_equal "3.1415926 (#{node_with_ref_without_name.id})", printable_name(node_with_ref_without_name) I18n.locale = "de" @@ -61,8 +61,8 @@ def test_printable_name assert_dom_equal "Test Node (#{node.id})", printable_name(node) assert_dom_equal "Test Node (#{node.id})", printable_name(node_v2) assert_dom_equal node.id.to_s, printable_name(node_v1) - assert_dom_equal "Test Node (#{node.id}, v2)", printable_name(node_v2, true) - assert_dom_equal "#{node.id}, v1", printable_name(node_v1, true) + assert_dom_equal "Test Node (#{node.id}, v2)", printable_name(node_v2, :version => true) + assert_dom_equal "#{node.id}, v1", printable_name(node_v1, :version => true) assert_dom_equal "3.1415926 (#{node_with_ref_without_name.id})", printable_name(node_with_ref_without_name) end diff --git a/test/models/diary_entry_test.rb b/test/models/diary_entry_test.rb index 76e8bbbe02..2b94d04dd7 100644 --- a/test/models/diary_entry_test.rb +++ b/test/models/diary_entry_test.rb @@ -8,18 +8,18 @@ def setup def test_diary_entry_validations diary_entry_valid({}) - diary_entry_valid({ :title => "" }, false) + diary_entry_valid({ :title => "" }, :valid => false) diary_entry_valid(:title => "a" * 255) - diary_entry_valid({ :title => "a" * 256 }, false) - diary_entry_valid({ :body => "" }, false) + diary_entry_valid({ :title => "a" * 256 }, :valid => false) + diary_entry_valid({ :body => "" }, :valid => false) diary_entry_valid(:latitude => 90) - diary_entry_valid({ :latitude => 90.00001 }, false) + diary_entry_valid({ :latitude => 90.00001 }, :valid => false) diary_entry_valid(:latitude => -90) - diary_entry_valid({ :latitude => -90.00001 }, false) + diary_entry_valid({ :latitude => -90.00001 }, :valid => false) diary_entry_valid(:longitude => 180) - diary_entry_valid({ :longitude => 180.00001 }, false) + diary_entry_valid({ :longitude => 180.00001 }, :valid => false) diary_entry_valid(:longitude => -180) - diary_entry_valid({ :longitude => -180.00001 }, false) + diary_entry_valid({ :longitude => -180.00001 }, :valid => false) end def test_diary_entry_visible @@ -48,8 +48,8 @@ def test_diary_entry_visible_comments private - def diary_entry_valid(attrs, result = true) + def diary_entry_valid(attrs, valid: true) entry = build(:diary_entry, attrs) - assert_equal result, entry.valid?, "Expected #{attrs.inspect} to be #{result}" + assert_equal valid, entry.valid?, "Expected #{attrs.inspect} to be #{valid}" end end diff --git a/test/models/trace_test.rb b/test/models/trace_test.rb index 8b38b7f0e2..fc5064ad7d 100644 --- a/test/models/trace_test.rb +++ b/test/models/trace_test.rb @@ -76,17 +76,17 @@ def test_tagged def test_validations trace_valid({}) - trace_valid({ :user_id => nil }, false) + trace_valid({ :user_id => nil }, :valid => false) trace_valid(:name => "a" * 255) - trace_valid({ :name => "a" * 256 }, false) - trace_valid({ :description => nil }, false) + trace_valid({ :name => "a" * 256 }, :valid => false) + trace_valid({ :description => nil }, :valid => false) trace_valid(:description => "a" * 255) - trace_valid({ :description => "a" * 256 }, false) + trace_valid({ :description => "a" * 256 }, :valid => false) trace_valid(:visibility => "private") trace_valid(:visibility => "public") trace_valid(:visibility => "trackable") trace_valid(:visibility => "identifiable") - trace_valid({ :visibility => "foo" }, false) + trace_valid({ :visibility => "foo" }, :valid => false) end def test_tagstring @@ -319,9 +319,9 @@ def check_xml_file(id, md5sum) assert_equal md5sum, md5sum(create(:trace, :fixture => id).xml_file) end - def trace_valid(attrs, result = true) + def trace_valid(attrs, valid: true) entry = build(:trace, attrs) - assert_equal result, entry.valid?, "Expected #{attrs.inspect} to be #{result}" + assert_equal valid, entry.valid?, "Expected #{attrs.inspect} to be #{valid}" end def md5sum(io) diff --git a/test/models/tracetag_test.rb b/test/models/tracetag_test.rb index 04263dbb74..1aaa596d2d 100644 --- a/test/models/tracetag_test.rb +++ b/test/models/tracetag_test.rb @@ -3,23 +3,23 @@ class TracetagTest < ActiveSupport::TestCase def test_validations tracetag_valid({}) - tracetag_valid({ :tag => nil }, false) - tracetag_valid({ :tag => "" }, false) + tracetag_valid({ :tag => nil }, :valid => false) + tracetag_valid({ :tag => "" }, :valid => false) tracetag_valid(:tag => "a") tracetag_valid(:tag => "a" * 255) - tracetag_valid({ :tag => "a" * 256 }, false) - tracetag_valid({ :tag => "a/b" }, false) - tracetag_valid({ :tag => "a;b" }, false) - tracetag_valid({ :tag => "a.b" }, false) - tracetag_valid({ :tag => "a,b" }, false) - tracetag_valid({ :tag => "a?b" }, false) + tracetag_valid({ :tag => "a" * 256 }, :valid => false) + tracetag_valid({ :tag => "a/b" }, :valid => false) + tracetag_valid({ :tag => "a;b" }, :valid => false) + tracetag_valid({ :tag => "a.b" }, :valid => false) + tracetag_valid({ :tag => "a,b" }, :valid => false) + tracetag_valid({ :tag => "a?b" }, :valid => false) end private - def tracetag_valid(attrs, result = true) + def tracetag_valid(attrs, valid: true) entry = build(:tracetag) entry.assign_attributes(attrs) - assert_equal result, entry.valid?, "Expected #{attrs.inspect} to be #{result}" + assert_equal valid, entry.valid?, "Expected #{attrs.inspect} to be #{valid}" end end From 78b9d92207bb215459ecff466c3efe2fa6466ab3 Mon Sep 17 00:00:00 2001 From: Andy Allan Date: Wed, 11 Nov 2020 16:14:24 +0000 Subject: [PATCH 2/2] Prefer keyword arguments when method has optional boolean arguments --- .rubocop_todo.yml | 3 --- app/controllers/api/notes_controller.rb | 4 ++-- app/controllers/application_controller.rb | 10 +++++----- app/controllers/browse_controller.rb | 2 +- app/controllers/changeset_comments_controller.rb | 2 +- app/controllers/changesets_controller.rb | 2 +- app/controllers/users_controller.rb | 2 +- app/helpers/browse_helper.rb | 2 +- app/views/browse/changeset.html.erb | 6 +++--- 9 files changed, 15 insertions(+), 18 deletions(-) diff --git a/.rubocop_todo.yml b/.rubocop_todo.yml index f6d11c58f9..67ef880c53 100644 --- a/.rubocop_todo.yml +++ b/.rubocop_todo.yml @@ -176,9 +176,6 @@ Style/NumericLiterals: # Offense count: 20 Style/OptionalBooleanParameter: Exclude: - - 'app/controllers/api/notes_controller.rb' - - 'app/controllers/application_controller.rb' - - 'app/helpers/browse_helper.rb' - 'app/models/changeset.rb' - 'app/models/node.rb' - 'app/models/relation.rb' diff --git a/app/controllers/api/notes_controller.rb b/app/controllers/api/notes_controller.rb index f4d1d0dac9..f480b97061 100644 --- a/app/controllers/api/notes_controller.rb +++ b/app/controllers/api/notes_controller.rb @@ -240,7 +240,7 @@ def destroy @note.status = "hidden" @note.save - add_comment(@note, comment, "hidden", false) + add_comment(@note, comment, "hidden", :notify => false) end # Return a copy of the updated note @@ -369,7 +369,7 @@ def closed_condition(notes) ## # Add a comment to a note - def add_comment(note, text, event, notify = true) + def add_comment(note, text, event, notify: true) attributes = { :visible => true, :event => event, :body => text } if current_user diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb index 052858932c..9f2d79eaa7 100644 --- a/app/controllers/application_controller.rb +++ b/app/controllers/application_controller.rb @@ -84,7 +84,7 @@ def require_cookies end end - def check_database_readable(need_api = false) + def check_database_readable(need_api: false) if Settings.status == "database_offline" || (need_api && Settings.status == "api_offline") if request.xhr? report_error "Database offline for maintenance", :service_unavailable @@ -94,7 +94,7 @@ def check_database_readable(need_api = false) end end - def check_database_writable(need_api = false) + def check_database_writable(need_api: false) if Settings.status == "database_offline" || Settings.status == "database_readonly" || (need_api && (Settings.status == "api_offline" || Settings.status == "api_readonly")) if request.xhr? @@ -171,7 +171,7 @@ def report_error(message, status = :bad_request) end end - def preferred_languages(reset = false) + def preferred_languages(reset: false) @preferred_languages = nil if reset @preferred_languages ||= if params[:locale] Locale.list(params[:locale]) @@ -184,13 +184,13 @@ def preferred_languages(reset = false) helper_method :preferred_languages - def set_locale(reset = false) + def set_locale(reset: false) if current_user&.languages&.empty? && !http_accept_language.user_preferred_languages.empty? current_user.languages = http_accept_language.user_preferred_languages current_user.save end - I18n.locale = Locale.available.preferred(preferred_languages(reset)) + I18n.locale = Locale.available.preferred(preferred_languages(:reset => reset)) response.headers["Vary"] = "Accept-Language" response.headers["Content-Language"] = I18n.locale.to_s diff --git a/app/controllers/browse_controller.rb b/app/controllers/browse_controller.rb index 1bbf85adce..50d0ae0a54 100644 --- a/app/controllers/browse_controller.rb +++ b/app/controllers/browse_controller.rb @@ -3,7 +3,7 @@ class BrowseController < ApplicationController before_action :authorize_web before_action :set_locale - before_action -> { check_database_readable(true) } + before_action -> { check_database_readable(:need_api => true) } before_action :require_oauth around_action :web_timeout authorize_resource :class => false diff --git a/app/controllers/changeset_comments_controller.rb b/app/controllers/changeset_comments_controller.rb index 4abffb90ef..0b5f6059a6 100644 --- a/app/controllers/changeset_comments_controller.rb +++ b/app/controllers/changeset_comments_controller.rb @@ -4,7 +4,7 @@ class ChangesetCommentsController < ApplicationController authorize_resource - before_action -> { check_database_readable(true) } + before_action -> { check_database_readable(:need_api => true) } around_action :web_timeout ## diff --git a/app/controllers/changesets_controller.rb b/app/controllers/changesets_controller.rb index 5b6d3e010d..7796dfeb27 100644 --- a/app/controllers/changesets_controller.rb +++ b/app/controllers/changesets_controller.rb @@ -6,7 +6,7 @@ class ChangesetsController < ApplicationController before_action :authorize_web before_action :set_locale - before_action -> { check_database_readable(true) }, :only => [:index, :feed] + before_action -> { check_database_readable(:need_api => true) }, :only => [:index, :feed] authorize_resource diff --git a/app/controllers/users_controller.rb b/app/controllers/users_controller.rb index 288fb2d5ca..7f12720a6e 100644 --- a/app/controllers/users_controller.rb +++ b/app/controllers/users_controller.rb @@ -652,7 +652,7 @@ def update_user(user, params) if user.save session[:fingerprint] = user.fingerprint - set_locale(true) + set_locale(:reset => true) if user.new_email.blank? || user.new_email == user.email flash.now[:notice] = t "users.account.flash update success" diff --git a/app/helpers/browse_helper.rb b/app/helpers/browse_helper.rb index 1e9465f803..0f533770b8 100644 --- a/app/helpers/browse_helper.rb +++ b/app/helpers/browse_helper.rb @@ -1,5 +1,5 @@ module BrowseHelper - def printable_name(object, version = false) + def printable_name(object, version: false) id = if object.id.is_a?(Array) object.id[0] else diff --git a/app/views/browse/changeset.html.erb b/app/views/browse/changeset.html.erb index 35cd4477b6..f58c465a74 100644 --- a/app/views/browse/changeset.html.erb +++ b/app/views/browse/changeset.html.erb @@ -90,7 +90,7 @@ <% end %> @@ -102,7 +102,7 @@ <% end %> @@ -114,7 +114,7 @@ <% end %>