[openstreetmap/openstreetmap-website] Allow more formats for internal latlon search (PR #5147)

[Tom Hughes]

@tomhughes commented on this pull request.



> @@ -57,6 +88,7 @@ def test_identify_latlon_ne_d
       "50.06773N, 14.37742E"
     ].each do |code|
       latlon_check code, 50.06773, 14.37742
+      assert_nil @controller.params[:latlon_digits]

Why do we want to assert this? The only thing this test should be concerned about is what results it gets from the controller method - how the controller does that internally is none of it's concern?

> -
-    ew = captures.fetch("ew").casecmp?("w") ? -1 : 1
-    ewd = BigDecimal(captures.fetch("ewd", "0"))
-    ewm = BigDecimal(captures.fetch("ewm", "0"))
-    ews = BigDecimal(captures.fetch("ews", "0"))
-
-    lat = ns * (nsd + (nsm / 60) + (nss / 3600))
-    lon = ew * (ewd + (ewm / 60) + (ews / 3600))
-
-    { :lat => lat.round(6).to_s("F"), :lon => lon.round(6).to_s("F") }
+  def dms_to_decdeg(prefix, directions, captures)
+    extract_number = ->(suffix) { captures.fetch("#{prefix}#{suffix}", "0").sub(",", ".") }
+
+    positive_direction, negative_direction = directions.chars
+    sign = captures.fetch(prefix, positive_direction).casecmp?(negative_direction) ? "-" : ""
+    deg = if captures["#{prefix}m"] || captures["#{prefix}s"]

Why do we need to special case the case when there are only degrees? What was wrong with the old way of doing it?

> +    [
+      "25.79° -80.27°",
+      "25.79°/-80.27°",
+      "25.79°, -80.27°",
+      "+25.79° -80.27°",
+      "+25.79°/-80.27°",
+      "+25.79°, -80.27°"
+    ].each do |code|
+      latlon_check code, 25.79, -80.27
+      assert @controller.params[:latlon_digits]
+    end
+  end
+
+  ##
+  # Test identification of basic lat/lon pairs with degrees/mins
+  def test_identify_latlon_basic_dm

Should we not have a dms version as well?

> -      if latlon = query.match(/^(?<ns>[NS])\s*#{dms_regexp('ns')}\W*(?<ew>[EW])\s*#{dms_regexp('ew')}$/) ||
-                  query.match(/^#{dms_regexp('ns')}\s*(?<ns>[NS])\W*#{dms_regexp('ew')}\s*(?<ew>[EW])$/)
-        params.merge!(to_decdeg(latlon.named_captures.compact)).delete(:query)
-
-      elsif latlon = query.match(%r{^(?<lat>[+-]?\d+(?:\.\d+)?)(?:\s+|\s*[,/]\s*)(?<lon>[+-]?\d+(?:\.\d+)?)$})
-        params.merge!(:lat => latlon["lat"], :lon => latlon["lon"]).delete(:query)
-
+      if match = query.match(/^(?<ns>[NS])\s*#{dms_regexp('ns')}\W*(?<ew>[EW])\s*#{dms_regexp('ew')}$/) ||
+                 query.match(/^#{dms_regexp('ns')}\s*(?<ns>[NS])\W*#{dms_regexp('ew')}\s*(?<ew>[EW])$/)
+        captures = match.named_captures.compact
+        params.merge! :lat => dms_to_decdeg("ns", "ns", captures),
+                      :lon => dms_to_decdeg("ew", "ew", captures)
+        params.delete(:query)
+
+      elsif match = query.match(%r{^
+                      (?<ns>[+-]?)\s*#{dms_regexp('ns', :comma => false)}

Why not allow commas in this case? Is it because it conflicts with the comma separating lat and lon? Having to exclude it in this case makes things a lot more ugly :-(

> -    ns = captures.fetch("ns").casecmp?("s") ? -1 : 1
-    nsd = BigDecimal(captures.fetch("nsd", "0"))
-    nsm = BigDecimal(captures.fetch("nsm", "0"))
-    nss = BigDecimal(captures.fetch("nss", "0"))
-
-    ew = captures.fetch("ew").casecmp?("w") ? -1 : 1
-    ewd = BigDecimal(captures.fetch("ewd", "0"))
-    ewm = BigDecimal(captures.fetch("ewm", "0"))
-    ews = BigDecimal(captures.fetch("ews", "0"))
-
-    lat = ns * (nsd + (nsm / 60) + (nss / 3600))
-    lon = ew * (ewd + (ewm / 60) + (ews / 3600))
-
-    { :lat => lat.round(6).to_s("F"), :lon => lon.round(6).to_s("F") }
+  def dms_to_decdeg(prefix, directions, captures)
+    extract_number = ->(suffix) { captures.fetch("#{prefix}#{suffix}", "0").sub(",", ".") }

Is this really better than just adding the `.sub` to the four fetches? I understand it's removing duplication but it does make the code much harder to read and understand.

-- 
Reply to this email directly or view it on GitHub:
https://github.com/openstreetmap/openstreetmap-website/pull/5147#pullrequestreview-2286679715
You are receiving this because you are subscribed to this thread.

Message ID: <openstreetmap/openstreetmap-website/pull/5147/review/2286679715 at github.com>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.openstreetmap.org/pipermail/rails-dev/attachments/20240906/2d48b1a2/attachment.htm>