Fix new rubocop warnings

.rubocop_todo.yml

@@ -1,6 +1,6 @@
 # This configuration was generated by
 # `rubocop --auto-gen-config`
-# on 2020-08-02 18:37:55 UTC using RuboCop version 0.86.0.
+# on 2020-08-06 17:20:38 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
@@ -39,13 +39,20 @@ Lint/AssignmentInCondition:
     - 'lib/osm.rb'
     - 'script/deliver-message'
 
-# Offense count: 739
+# Offense count: 20
+Lint/MissingSuper:
+  Exclude:
+    - 'config/initializers/oauth.rb'
+    - 'lib/osm.rb'
+    - 'lib/session_persistence.rb'
+
+# Offense count: 682
 # Configuration parameters: IgnoredMethods.
 Metrics/AbcSize:
-  Max: 189
+  Max: 194
 
 # Offense count: 72
-# Configuration parameters: CountComments, ExcludedMethods.
+# Configuration parameters: CountComments, CountAsOne, ExcludedMethods.
 # ExcludedMethods: refine
 Metrics/BlockLength:
   Max: 71
@@ -56,17 +63,17 @@ Metrics/BlockNesting:
   Max: 5
 
 # Offense count: 25
-# Configuration parameters: CountComments.
+# Configuration parameters: CountComments, CountAsOne.
 Metrics/ClassLength:
   Max: 645
 
 # Offense count: 68
 # Configuration parameters: IgnoredMethods.
 Metrics/CyclomaticComplexity:
-  Max: 28
+  Max: 26
 
 # Offense count: 735
-# Configuration parameters: CountComments, ExcludedMethods.
+# Configuration parameters: CountComments, CountAsOne, ExcludedMethods.
 Metrics/MethodLength:
   Max: 179
 
@@ -75,10 +82,10 @@ Metrics/MethodLength:
 Metrics/ParameterLists:
   Max: 9
 
-# Offense count: 73
+# Offense count: 69
 # Configuration parameters: IgnoredMethods.
 Metrics/PerceivedComplexity:
-  Max: 25
+  Max: 26
 
 # Offense count: 6
 Naming/AccessorMethodName:
@@ -161,6 +168,15 @@ Rails/OutputSafety:
 Rails/TimeZone:
   Enabled: false
 
+# Offense count: 6
+# Cop supports --auto-correct.
+Style/ExplicitBlockArgument:
+  Exclude:
+    - 'app/controllers/api/amf_controller.rb'
+    - 'app/controllers/application_controller.rb'
+    - 'app/mailers/notifier.rb'
+    - 'test/integration/client_applications_test.rb'
+
 # Offense count: 572
 # Cop supports --auto-correct.
 # Configuration parameters: EnforcedStyle.
@@ -173,3 +189,24 @@ Style/FrozenStringLiteralComment:
 # Configuration parameters: Strict.
 Style/NumericLiterals:
   MinDigits: 11
+
+# 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'
+    - '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: 44
+# Cop supports --auto-correct.
+Style/StringConcatenation:
+  Enabled: false

Gemfile

@@ -41,8 +41,8 @@ gem "image_optim_rails"
 
 # Load rails plugins
 gem "actionpack-page_caching", ">= 1.2.0"
-gem "active_record_union"
 gem "activerecord-import"
+gem "active_record_union"
 gem "bootstrap", "~> 4.5.0"
 gem "bootstrap_form", "~> 4.0"
 gem "cancancan"

app/controllers/application_controller.rb

@@ -11,8 +11,7 @@ class ApplicationController < ActionController::Base
   before_action :fetch_body
   around_action :better_errors_allow_inline, :if => proc { Rails.env.development? }
 
-  attr_accessor :current_user
+  attr_accessor :current_user, :oauth_token
-  attr_accessor :oauth_token
 
   helper_method :current_user
   helper_method :oauth_token
@@ -115,9 +114,10 @@ class ApplicationController < ActionController::Base
   end
 
   def database_status
-    if Settings.status == "database_offline"
+    case Settings.status
+    when "database_offline"
       "offline"
-    elsif Settings.status == "database_readonly"
+    when "database_readonly"
       "readonly"
     else
       "online"
@@ -127,9 +127,10 @@ class ApplicationController < ActionController::Base
   def api_status
     status = database_status
     if status == "online"
-      if Settings.status == "api_offline"
+      case Settings.status
+      when "api_offline"
         status = "offline"
-      elsif Settings.status == "api_readonly"
+      when "api_readonly"
         status = "readonly"
       end
     end
@@ -288,9 +289,10 @@ class ApplicationController < ActionController::Base
       :style_src => %w['unsafe-inline']
     )
 
-    if Settings.status == "database_offline" || Settings.status == "api_offline"
+    case Settings.status
+    when "database_offline", "api_offline"
       flash.now[:warning] = t("layouts.osm_offline")
-    elsif Settings.status == "database_readonly" || Settings.status == "api_readonly"
+    when "database_readonly", "api_readonly"
       flash.now[:warning] = t("layouts.osm_read_only")
     end
 
@@ -302,15 +304,13 @@ class ApplicationController < ActionController::Base
   end
 
   def preferred_editor
-    editor = if params[:editor]
+    if params[:editor]
-               params[:editor]
+      params[:editor]
-             elsif current_user&.preferred_editor
+    elsif current_user&.preferred_editor
-               current_user.preferred_editor
+      current_user.preferred_editor
-             else
+    else
-               Settings.default_editor
+      Settings.default_editor
-             end
+    end
-
-    editor
   end
 
   helper_method :preferred_editor

app/controllers/browse_controller.rb

@@ -18,7 +18,7 @@ class BrowseController < ApplicationController
 
   def relation_history
     @type = "relation"
-    @feature = Relation.preload(:relation_tags, :old_relations => [:old_tags, :changeset => [:changeset_tags, :user], :old_members => :member]).find(params[:id])
+    @feature = Relation.preload(:relation_tags, :old_relations => [:old_tags, { :changeset => [:changeset_tags, :user], :old_members => :member }]).find(params[:id])
     render "history"
   rescue ActiveRecord::RecordNotFound
     render :action => "not_found", :status => :not_found
@@ -26,7 +26,7 @@ class BrowseController < ApplicationController
 
   def way
     @type = "way"
-    @feature = Way.preload(:way_tags, :containing_relation_members, :changeset => [:changeset_tags, :user], :nodes => [:node_tags, :ways => :way_tags]).find(params[:id])
+    @feature = Way.preload(:way_tags, :containing_relation_members, :changeset => [:changeset_tags, :user], :nodes => [:node_tags, { :ways => :way_tags }]).find(params[:id])
     render "feature"
   rescue ActiveRecord::RecordNotFound
     render :action => "not_found", :status => :not_found
@@ -34,7 +34,7 @@ class BrowseController < ApplicationController
 
   def way_history
     @type = "way"
-    @feature = Way.preload(:way_tags, :old_ways => [:old_tags, :changeset => [:changeset_tags, :user], :old_nodes => { :node => [:node_tags, :ways] }]).find(params[:id])
+    @feature = Way.preload(:way_tags, :old_ways => [:old_tags, { :changeset => [:changeset_tags, :user], :old_nodes => { :node => [:node_tags, :ways] } }]).find(params[:id])
     render "history"
   rescue ActiveRecord::RecordNotFound
     render :action => "not_found", :status => :not_found
@@ -50,7 +50,7 @@ class BrowseController < ApplicationController
 
   def node_history
     @type = "node"
-    @feature = Node.preload(:node_tags, :old_nodes => [:old_tags, :changeset => [:changeset_tags, :user]]).find(params[:id])
+    @feature = Node.preload(:node_tags, :old_nodes => [:old_tags, { :changeset => [:changeset_tags, :user] }]).find(params[:id])
     render "history"
   rescue ActiveRecord::RecordNotFound
     render :action => "not_found", :status => :not_found

app/controllers/export_controller.rb

@@ -11,11 +11,12 @@ class ExportController < ApplicationController
     bbox = BoundingBox.from_lon_lat_params(params)
     format = params[:format]
 
-    if format == "osm"
+    case format
+    when "osm"
       # redirect to API map get
       redirect_to :controller => "api/map", :action => "index", :bbox => bbox
 
-    elsif format == "mapnik"
+    when "mapnik"
       # redirect to a special 'export' cgi script
       format = params[:mapnik_format]
       scale = params[:mapnik_scale]

app/controllers/geocoder_controller.rb

@@ -17,11 +17,12 @@ class GeocoderController < ApplicationController
       @sources.push "osm_nominatim_reverse"
       @sources.push "geonames_reverse" if Settings.key?(:geonames_username)
     elsif @params[:query]
-      if @params[:query].match?(/^\d{5}(-\d{4})?$/)
+      case @params[:query]
+      when /^\d{5}(-\d{4})?$/
         @sources.push "osm_nominatim"
-      elsif @params[:query].match?(/^(GIR 0AA|[A-PR-UWYZ]([0-9]{1,2}|([A-HK-Y][0-9]|[A-HK-Y][0-9]([0-9]|[ABEHMNPRV-Y]))|[0-9][A-HJKS-UW])\s*[0-9][ABD-HJLNP-UW-Z]{2})$/i)
+      when /^(GIR 0AA|[A-PR-UWYZ]([0-9]{1,2}|([A-HK-Y][0-9]|[A-HK-Y][0-9]([0-9]|[ABEHMNPRV-Y]))|[0-9][A-HJKS-UW])\s*[0-9][ABD-HJLNP-UW-Z]{2})$/i
         @sources.push "osm_nominatim"
-      elsif @params[:query].match?(/^[A-Z]\d[A-Z]\s*\d[A-Z]\d$/i)
+      when /^[A-Z]\d[A-Z]\s*\d[A-Z]\d$/i
         @sources.push "ca_postcode"
         @sources.push "osm_nominatim"
       else

app/helpers/browse_tags_helper.rb

@@ -60,7 +60,8 @@ module BrowseTagsHelper
     # Some k/v's are wikipedia=http://en.wikipedia.org/wiki/Full%20URL
     return nil if %r{^https?://}.match?(value)
 
-    if key == "wikipedia"
+    case key
+    when "wikipedia"
       # This regex should match Wikipedia language codes, everything
       # from de to zh-classical
       lang = if value =~ /^([a-z-]{2,12}):(.+)$/i
@@ -71,7 +72,7 @@ module BrowseTagsHelper
                # Value is <title> so default to English Wikipedia
                "en"
              end
-    elsif key =~ /^wikipedia:(\S+)$/
+    when /^wikipedia:(\S+)$/
       # Language is in the key, so assume value is the title
       lang = Regexp.last_match(1)
     else

app/models/changeset.rb

@@ -83,11 +83,13 @@ class Changeset < ApplicationRecord
   def self.from_xml(xml, create = false)
     p = XML::Parser.string(xml, :options => XML::Parser::Options::NOERROR)
     doc = p.parse
+    pt = doc.find_first("//osm/changeset")
 
-    doc.find("//osm/changeset").each do |pt|
+    if pt
-      return Changeset.from_xml_node(pt, create)
+      Changeset.from_xml_node(pt, create)
+    else
+      raise OSM::APIBadXMLError.new("changeset", xml, "XML doesn't contain an osm/changeset element.")
     end
-    raise OSM::APIBadXMLError.new("changeset", xml, "XML doesn't contain an osm/changeset element.")
   rescue LibXML::XML::Error, ArgumentError => e
     raise OSM::APIBadXMLError.new("changeset", xml, e.message)
   end

app/models/client_application.rb

@@ -56,8 +56,7 @@ class ClientApplication < ApplicationRecord
     signature = OAuth::Signature.build(request, options, &block)
     return false unless OauthNonce.remember(signature.request.nonce, signature.request.timestamp)
 
-    value = signature.verify
+    signature.verify
-    value
   rescue OAuth::Signature::UnknownSignatureMethod
     false
   end

app/models/node.rb

@@ -74,11 +74,13 @@ class Node < ApplicationRecord
   def self.from_xml(xml, create = false)
     p = XML::Parser.string(xml, :options => XML::Parser::Options::NOERROR)
     doc = p.parse
+    pt = doc.find_first("//osm/node")
 
-    doc.find("//osm/node").each do |pt|
+    if pt
-      return Node.from_xml_node(pt, create)
+      Node.from_xml_node(pt, create)
+    else
+      raise OSM::APIBadXMLError.new("node", xml, "XML doesn't contain an osm/node element.")
     end
-    raise OSM::APIBadXMLError.new("node", xml, "XML doesn't contain an osm/node element.")
   rescue LibXML::XML::Error, ArgumentError => e
     raise OSM::APIBadXMLError.new("node", xml, e.message)
   end

app/models/old_relation.rb

@@ -86,9 +86,7 @@ class OldRelation < ApplicationRecord
     @tags ||= Hash[old_tags.collect { |t| [t.k, t.v] }]
   end
 
-  attr_writer :members
+  attr_writer :members, :tags
-
-  attr_writer :tags
 
   def to_xml
     doc = OSM::API.new.get_xml_doc

app/models/old_way.rb

@@ -84,9 +84,7 @@ class OldWay < ApplicationRecord
     @tags ||= Hash[old_tags.collect { |t| [t.k, t.v] }]
   end
 
-  attr_writer :nds
+  attr_writer :nds, :tags
-
-  attr_writer :tags
 
   def to_xml_node(changeset_cache = {}, user_display_name_cache = {})
     el = XML::Node.new "way"

app/models/relation.rb

@@ -57,11 +57,13 @@ class Relation < ApplicationRecord
   def self.from_xml(xml, create = false)
     p = XML::Parser.string(xml, :options => XML::Parser::Options::NOERROR)
     doc = p.parse
+    pt = doc.find_first("//osm/relation")
 
-    doc.find("//osm/relation").each do |pt|
+    if pt
-      return Relation.from_xml_node(pt, create)
+      Relation.from_xml_node(pt, create)
+    else
+      raise OSM::APIBadXMLError.new("node", xml, "XML doesn't contain an osm/relation element.")
     end
-    raise OSM::APIBadXMLError.new("node", xml, "XML doesn't contain an osm/relation element.")
   rescue LibXML::XML::Error, ArgumentError => e
     raise OSM::APIBadXMLError.new("relation", xml, e.message)
   end
@@ -132,9 +134,7 @@ class Relation < ApplicationRecord
     @tags ||= Hash[relation_tags.collect { |t| [t.k, t.v] }]
   end
 
-  attr_writer :members
+  attr_writer :members, :tags
-
-  attr_writer :tags
 
   def add_member(type, id, role)
     @members ||= []

app/models/trace.rb

@@ -125,19 +125,17 @@ class Trace < ApplicationRecord
     zipped = filetype.include?("Zip archive")
     tarred = filetype.include?("tar archive")
 
-    mimetype = if gzipped
+    if gzipped
-                 "application/x-gzip"
+      "application/x-gzip"
-               elsif bzipped
+    elsif bzipped
-                 "application/x-bzip2"
+      "application/x-bzip2"
-               elsif zipped
+    elsif zipped
-                 "application/x-zip"
+      "application/x-zip"
-               elsif tarred
+    elsif tarred
-                 "application/x-tar"
+      "application/x-tar"
-               else
+    else
-                 "application/gpx+xml"
+      "application/gpx+xml"
-               end
+    end
-
-    mimetype
   end
 
   def extension_name
@@ -147,34 +145,33 @@ class Trace < ApplicationRecord
     zipped = filetype.include?("Zip archive")
     tarred = filetype.include?("tar archive")
 
-    extension = if tarred && gzipped
+    if tarred && gzipped
-                  ".tar.gz"
+      ".tar.gz"
-                elsif tarred && bzipped
+    elsif tarred && bzipped
-                  ".tar.bz2"
+      ".tar.bz2"
-                elsif tarred
+    elsif tarred
-                  ".tar"
+      ".tar"
-                elsif gzipped
+    elsif gzipped
-                  ".gpx.gz"
+      ".gpx.gz"
-                elsif bzipped
+    elsif bzipped
-                  ".gpx.bz2"
+      ".gpx.bz2"
-                elsif zipped
+    elsif zipped
-                  ".zip"
+      ".zip"
-                else
+    else
-                  ".gpx"
+      ".gpx"
-                end
+    end
-
-    extension
   end
 
   def update_from_xml(xml, create = false)
     p = XML::Parser.string(xml, :options => XML::Parser::Options::NOERROR)
     doc = p.parse
+    pt = doc.find_first("//osm/gpx_file")
 
-    doc.find("//osm/gpx_file").each do |pt|
+    if pt
-      return update_from_xml_node(pt, create)
+      update_from_xml_node(pt, create)
+    else
+      raise OSM::APIBadXMLError.new("trace", xml, "XML doesn't contain an osm/gpx_file element.")
     end
-
-    raise OSM::APIBadXMLError.new("trace", xml, "XML doesn't contain an osm/gpx_file element.")
   rescue LibXML::XML::Error, ArgumentError => e
     raise OSM::APIBadXMLError.new("trace", xml, e.message)
   end

app/models/way.rb

@@ -55,11 +55,13 @@ class Way < ApplicationRecord
   def self.from_xml(xml, create = false)
     p = XML::Parser.string(xml, :options => XML::Parser::Options::NOERROR)
     doc = p.parse
+    pt = doc.find_first("//osm/way")
 
-    doc.find("//osm/way").each do |pt|
+    if pt
-      return Way.from_xml_node(pt, create)
+      Way.from_xml_node(pt, create)
+    else
+      raise OSM::APIBadXMLError.new("node", xml, "XML doesn't contain an osm/way element.")
     end
-    raise OSM::APIBadXMLError.new("node", xml, "XML doesn't contain an osm/way element.")
   rescue LibXML::XML::Error, ArgumentError => e
     raise OSM::APIBadXMLError.new("way", xml, e.message)
   end
@@ -114,9 +116,7 @@ class Way < ApplicationRecord
     @tags ||= Hash[way_tags.collect { |t| [t.k, t.v] }]
   end
 
-  attr_writer :nds
+  attr_writer :nds, :tags
-
-  attr_writer :tags
 
   def add_nd_num(n)
     @nds ||= []

app/views/site/key.html.erb

@@ -7,7 +7,7 @@
             <%= image_tag "key/#{name}/#{entry['image']}" %>
           </td>
           <td class="mapkey-table-value">
-            <%= [*t(".table.entry.#{entry['name']}")].to_sentence %>
+            <%= Array(t(".table.entry.#{entry['name']}")).to_sentence %>
           </td>
         </tr>
       <% end %>

config/environments/production.rb

@@ -84,7 +84,7 @@ Rails.application.configure do
   # config.logger = ActiveSupport::TaggedLogging.new(Syslog::Logger.new 'app-name')
 
   if ENV["RAILS_LOG_TO_STDOUT"].present?
-    logger           = ActiveSupport::Logger.new(STDOUT)
+    logger           = ActiveSupport::Logger.new($stdout)
     logger.formatter = config.log_formatter
     config.logger    = ActiveSupport::TaggedLogging.new(logger)
   end

lib/classic_pagination/pagination.rb

@@ -394,7 +394,7 @@ module ActionController
           @page = page
           self.padding = padding
         end
-        attr_reader :paginator, :page
+        attr_reader :paginator, :page, :padding, :first, :last
 
         # Sets the window's padding (the number of pages on either side of the
         # window page).
@@ -412,7 +412,6 @@ module ActionController
                     @paginator.last
                   end
         end
-        attr_reader :padding, :first, :last
 
         # Returns an array of Page objects in the current window.
         def pages

lib/diff_reader.rb

@@ -133,7 +133,8 @@ class DiffReader
 
     # loop at the top level, within the <osmChange> element
     with_element do |action_name, action_attributes|
-      if action_name == "create"
+      case action_name
+      when "create"
         # create a new element. this code is agnostic of the element type
         # because all the elements support the methods that we're using.
         with_model do |model, xml|
@@ -168,7 +169,7 @@ class DiffReader
           result.root << xml_result
         end
 
-      elsif action_name == "modify"
+      when "modify"
         # modify an existing element. again, this code doesn't directly deal
         # with types, but uses duck typing to handle them transparently.
         with_model do |model, xml|
@@ -200,7 +201,7 @@ class DiffReader
           result.root << xml_result
         end
 
-      elsif action_name == "delete"
+      when "delete"
         # delete action. this takes a payload in API 0.6, so we need to do
         # most of the same checks that are done for the modify.
         with_model do |model, xml|

lib/gpx.rb

@@ -4,9 +4,7 @@ module GPX
 
     include LibXML
 
-    attr_reader :possible_points
+    attr_reader :possible_points, :actual_points, :tracksegs
-    attr_reader :actual_points
-    attr_reader :tracksegs
 
     def initialize(file)
       @file = file
@@ -16,7 +14,8 @@ module GPX
       point = nil
 
       while reader.read
-        if reader.node_type == XML::Reader::TYPE_ELEMENT
+        case reader.node_type
+        when XML::Reader::TYPE_ELEMENT
           if reader.name == "trkpt"
             point = TrkPt.new(@tracksegs, reader["lat"].to_f, reader["lon"].to_f)
             @possible_points += 1
@@ -25,7 +24,7 @@ module GPX
           elsif reader.name == "time" && point
             point.timestamp = Time.parse(reader.read_string)
           end
-        elsif reader.node_type == XML::Reader::TYPE_END_ELEMENT
+        when XML::Reader::TYPE_END_ELEMENT
           if reader.name == "trkpt" && point && point.valid?
             point.altitude ||= 0
             yield point

lib/potlatch.rb

@@ -144,7 +144,7 @@ module Potlatch
 
       # Output response header
       a, b = bodies.divmod(256)
-      yield 0.chr + 0.chr + 0.chr + 0.chr + a.chr + b.chr
+      yield 0.chr * 4 + a.chr + b.chr
 
       # Process the bodies
       bodies.times do                     # Read each body

script/deliver-message

@@ -25,7 +25,7 @@ exit 0 if date < 1.month.ago
 
 message&.update(:message_read => true)
 
-mail = Mail.new(STDIN.read
+mail = Mail.new($stdin.read
                      .encode(:universal_newline => true)
                      .encode(:crlf_newline => true))
 

test/controllers/api/amf_controller_test.rb

@@ -1390,8 +1390,8 @@ module Api
     def amf_content(target, ref, data)
       a, b = 1.divmod(256)
       c = StringIO.new
-      c.write 0.chr + 0.chr   # version 0
+      c.write 0.chr * 2       # version 0
-      c.write 0.chr + 0.chr   # n headers
+      c.write 0.chr * 2       # n headers
       c.write a.chr + b.chr   # n bodies
       c.write AMF.encodestring(target)
       c.write AMF.encodestring(ref)

test/lib/i18n_test.rb

@@ -63,13 +63,14 @@ class I18nTest < ActiveSupport::TestCase
     I18n.t(scope || ".", :locale => I18n.default_locale).map do |key, value|
       scoped_key = scope ? "#{scope}.#{key}" : key
 
-      if value.is_a?(Hash)
+      case value
+      when Hash
         if value.keys - plural_keys == []
           scoped_key
         else
           translation_keys(scoped_key)
         end
-      elsif value.is_a?(String)
+      when String
         scoped_key
       end
     end.flatten