<p><b>@gravitystorm</b> requested changes on this pull request.</p>

<p>Again, all looks good to me, just a couple of minor points inline.</p>
<p>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.     <code>before_action :set_default_request_format, :except => [:create, :update, :delete]</code>). Can you talk more about that?</p><hr>

<p>In <a href="https://github.com/openstreetmap/openstreetmap-website/pull/2485#discussion_r369560194">app/controllers/api_controller.rb</a>:</p>
<pre style='color:#555'>> @@ -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
</pre>
<p>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.</p>
<p>The edge-case is that if the header exists, and is parseable, but doesn't contain either xml, json, or '<em>/</em>', then request.format isn't set. I'm not sure if that has any consequences though.</p>
<p>I think it could be simpler if you assume request.format = "xml" on the first line of the function. Then e.g. just <code>return if accept_header.nil?</code> would be enough.</p>
<p>There's a few options for refactoring the main loop here, to avoid having to use <code>break</code>, which is unusual to see in ruby code. For example, you could use <code>.map</code> to convert every entry into one of ["xml", "json", nil] and then <code>.compact.first</code> 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.</p>

<hr>

<p>In <a href="https://github.com/openstreetmap/openstreetmap-website/pull/2485#discussion_r369564106">app/views/api/nodes/index.json.jbuilder</a>:</p>
<pre style='color:#555'>> @@ -0,0 +1,5 @@
+json.partial! "api/map/root_attributes"
</pre>
<p>Shared partials should be in a shared folder - see <a href="https://edgeguides.rubyonrails.org/layouts_and_rendering.html#template-inheritance" rel="nofollow">https://edgeguides.rubyonrails.org/layouts_and_rendering.html#template-inheritance</a></p>
<p>I haven't tested it, but I think <code>app/views/api</code> will be the first-level fallback, or <code>app/views/application</code> as the default.</p>

<hr>

<p>In <a href="https://github.com/openstreetmap/openstreetmap-website/pull/2485#discussion_r369565818">test/controllers/api/map_controller_test.rb</a>:</p>
<pre style='color:#555'>> @@ -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)
</pre>
<p>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.</p>

<p style="font-size:small;-webkit-text-size-adjust:none;color:#666;">—<br />You are receiving this because you are subscribed to this thread.<br />Reply to this email directly, <a href="https://github.com/openstreetmap/openstreetmap-website/pull/2485?email_source=notifications&email_token=AAK2OLOFXDT5JCC6X4BL5NDQ7BG4RA5CNFSM4KAKS5KKYY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCSUIE4A#pullrequestreview-346587760">view it on GitHub</a>, or <a href="https://github.com/notifications/unsubscribe-auth/AAK2OLM7K3Z4RJKEJ7SJ2KTQ7BG4RANCNFSM4KAKS5KA">unsubscribe</a>.<img src="https://github.com/notifications/beacon/AAK2OLPXK2MFJZY6R22VJSLQ7BG4RA5CNFSM4KAKS5KKYY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCSUIE4A.gif" height="1" width="1" alt="" /></p>
<script type="application/ld+json">[
{
"@context": "http://schema.org",
"@type": "EmailMessage",
"potentialAction": {
"@type": "ViewAction",
"target": "https://github.com/openstreetmap/openstreetmap-website/pull/2485?email_source=notifications\u0026email_token=AAK2OLOFXDT5JCC6X4BL5NDQ7BG4RA5CNFSM4KAKS5KKYY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCSUIE4A#pullrequestreview-346587760",
"url": "https://github.com/openstreetmap/openstreetmap-website/pull/2485?email_source=notifications\u0026email_token=AAK2OLOFXDT5JCC6X4BL5NDQ7BG4RA5CNFSM4KAKS5KKYY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCSUIE4A#pullrequestreview-346587760",
"name": "View Pull Request"
},
"description": "View this Pull Request on GitHub",
"publisher": {
"@type": "Organization",
"name": "GitHub",
"url": "https://github.com"
}
}
]</script>