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

[Andy Allan]

gravitystorm requested changes on this pull request.

Again, all looks good to me, just a couple of minor points inline.

The bigger question is around which methods are covered by this PR, and which methods aren't. For example, I think the set_default_request_format would be better applied to every api method, but you only have it for a few (e.g.     `before_action :set_default_request_format, :except => [:create, :update, :delete]`). Can you talk more about that?

> @@ -3,6 +3,52 @@ class ApiController < ApplicationController
 
   private
 
+  ##
+  # Set default request format to xml unless a client requests a specific format,
+  # which can be done via (a) URL suffix and/or (b) HTTP Accept header, where
+  # the URL suffix always takes precedence over the Accept header.
+  def set_default_request_format

I know that you've revised this method a few times, so I'm hesitant to suggest more changes! But there's some non-idiomatic code here, and a small edge-case too.

The edge-case is that if the header exists, and is parseable, but doesn't contain either xml, json, or '*/*', then request.format isn't set. I'm not sure if that has any consequences though.

I think it could be simpler if you assume request.format = "xml" on the first line of the function. Then e.g. just `return if accept_header.nil?` would be enough.

There's a few options for refactoring the main loop here, to avoid having to use `break`, which is unusual to see in ruby code. For example, you could use `.map` to convert every entry into one of ["xml", "json", nil] and then `.compact.first` to get the first non-nil result, and use that for the request.format. Or alternative approaches - there's a lot of useful methods for Arrays and Enumerables in ruby. Of course, if you've had enough of refactoring this function already, I understand.

> @@ -0,0 +1,5 @@
+json.partial! "api/map/root_attributes"

Shared partials should be in a shared folder - see https://edgeguides.rubyonrails.org/layouts_and_rendering.html#template-inheritance

I haven't tested it, but I think `app/views/api` will be the first-level fallback, or `app/views/application` as the default.

> @@ -23,6 +23,82 @@ def test_routes
         { :path => "/api/0.6/map", :method => :get },
         { :controller => "api/map", :action => "index" }
       )
+      assert_routing(
+        { :path => "/api/0.6/map.json", :method => :get },
+        { :controller => "api/map", :action => "index", :format => "json" }
+      )
+    end
+
+    ##
+    # test http accept headers
+    def test_http_accept_header
+      node = create(:node, :lat => 7, :lon => 7)

Don't add specific lats and lons unless they are important - the factory will set defaults. The 7 is important for testing floating point format outputs, iirc, but that's not happening here.

-- 
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-346587760
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.openstreetmap.org/pipermail/rails-dev/attachments/20200122/040cf0a9/attachment.htm>