Re: [openstreetmap/openstreetmap-website] [WIP] XML generation: nodes, ways, relations, map controller → view (#2223)
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
... 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:
-------------- next part --------------
An HTML attachment was scrubbed...
More information about the rails-dev