[openstreetmap/openstreetmap-website] JSON output nodes, ways, relations, map (#2485)

Andy Allan notifications at github.com
Mon Dec 30 16:19:29 UTC 2019


gravitystorm commented on this pull request.

This looks great! Just a few comments from me based on an initial look through the PR. I plan to look at this in more detail in the next few weeks.

> @@ -5,6 +5,13 @@ class MapController < ApiController
     before_action :check_api_readable
     around_action :api_call_handle_error, :api_call_timeout
 
+    before_action :default_format_xml
+
+    # Set format to xml unless client requires a specific format
+    def default_format_xml

There's no need to repeat the function definition in each controller. You can move the definition to `app/controllers/api_controller.rb`, since all the controllers in `app/controllers/api/` are subclassed from that one.

> @@ -5,6 +5,13 @@ class MapController < ApiController
     before_action :check_api_readable
     around_action :api_call_handle_error, :api_call_timeout
 
+    before_action :default_format_xml

We need to be careful about this. Previously in #2065 we ran into problems with accidentally ignoring `Accepts` headers. This approach is different, so perhaps avoids that problem, but we should have test-suite coverage for whatever we are trying to support.

> @@ -0,0 +1,15 @@
+json.type "node"
+json.id node.id
+if node.visible
+  json.lat format("%.7f", node.lat.to_f)

In order to get the XML output the way we want it, we have created our own "Coord" class. This allows us to precisely control what the output is whenever the coord is converted to a string, without having to write "format(%.7f)" all over the codebase.

https://github.com/openstreetmap/openstreetmap-website/blob/c524cbf096d9f67936a5ec6050e5c83fd49077cd/app/models/concerns/geo_record.rb#L4-L15

In that PR I used `.to_f` in the json views for notes, to avoid the builder calling `.to_s` and getting quoted strings. Would that avoid the quoting problem you have here too?

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/openstreetmap/openstreetmap-website/pull/2485#pullrequestreview-337154722
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.openstreetmap.org/pipermail/rails-dev/attachments/20191230/b9d090c8/attachment.html>


More information about the rails-dev mailing list