Re: [openstreetmap/openstreetmap-website] [WIP] XML generation: nodes, ways, relations, map controller → view (#2223)

Andy Allan notifications at github.com
Wed Jun 26 13:41:47 UTC 2019


Sorry for not providing feedback more quickly. I've had a look at this, and here are my suggestions:

* Avoid the topic of default rendering formats in this PR. It creates a breaking API change in some edge cases (as per #2065) so best dealt with separately. This means your controllers shouldn't contain `respond_to` blocks and the tests then won't need changes.

* Don't wrap objects in arrays unnecessarily. For example `@nodes = [node]`. The relevant view should just handle an `@node` object instead.

* Try to avoid creating arrays for no reason. For example, after refactoring your code becomes:

```
      @elems = []

      visible_elements.each do |element|
        @elems << element
      end
```
... which isn't doing anything useful!

* If you're not sure how to handle the caches for users etc (and to be honest, I don't know either) then just leave these methods and come back to them in a separate PR. I'd prefer to see one PR that covers all the straightforward refactoring and is easy to review, and a second PR that covers tough things but is nice and short.

* I think there's need for `.empty?` guards when using `.each` (in  app/views/api/map/_node.xml.builder)

* Avoid using `@variables` in partials. The partials are easier to reuse/understand if you pass in the objects at the point they are rendered. For example, you have `@bounds` in the _bounds.builder partial, but it would be better to call it from the index.xml.builder with e.g. `  osm << (render(:partial => "bounds", :object => @bounds) || "")` and then just use `bounds` in the bounds partial.

* Check out https://github.com/openstreetmap/openstreetmap-website/pull/2278 since it applies to many of the partial renderings that you have in this PR.

* Do you really need `|| ""` in all these render calls? I suspect (but haven't tested) that most calls will work without them.


-- 
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/2223#issuecomment-505880111
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.openstreetmap.org/pipermail/rails-dev/attachments/20190626/daf50696/attachment.html>


More information about the rails-dev mailing list