ccocchi / rabl-rails

Rails 4.2+ templating system with JSON, XML and Plist support.
MIT License
208 stars 51 forks source link

Allow nodes with `nil` key to get merged into main #13

Closed fnordfish closed 11 years ago

fnordfish commented 11 years ago

This is like calling extends but changing the inner object.

Example:

collection :@child_objects

node(nil) { |child_object| partial 'parents_detail', object: child_object.my_parent }
onyxraven commented 11 years ago

I need this functionality.

rabl has a code function that essentially does the same thing.

Similarly, I'd like to be able to use glue with the current object instead of having to give it a name, or document the name of the 'current' object in a partial or collection

ccocchi commented 11 years ago

This can already be achieved via glue

collection :@child_objects

node(nil) { |child_object| partial 'parents_detail', object: child_object.my_parent }

is equivalent to

collection :@child_objects

glue(:my_parent) { extends('parents_detail') }
ccocchi commented 11 years ago

@fnordfish does the solution with glue answer your problem ? Or can we close this issue.

@onyxraven I don't really understand what you trying to achieve. Do you have a concrete example? (you can open a new issue if it's not related to this one?)

fnordfish commented 11 years ago

@ccocchi, Thanks for your sample! glue was actually my first try, but somehow I did not get it to work (I also need some conditions on those nodes, which makes it a bit more complex). However, I'll try your approach and let you know.

Thanks again!

ccocchi commented 11 years ago

You may use the condition keyword for that, for example

collection :@children

condition(->(c) { c.has_parent? }) do
  glue(:parent) { extends('parents_details') }
end
fnordfish commented 11 years ago

This didn't turn out pretty well :( It seems that glue does not cope with node in the extended template.

# children.json.rabl
collection :@children, root: 'children', object_root: false
glue(:parent) { extends('parents_details') }
node(:foo)

and

# parents_details.json.rabl
object :@parent => :parent
attributes :id
node(:foo) { "42" }

does not work. rabl-rails will crash with something like <Proc:0x00000005a290b8@(eval):5> is not a symbol when it tries to glue in the node(:foo).

See my PR #20. Using those fix, it seems to work fine - I have not yet refactored all those node(nil) into glue but looks pretty promising.

fnordfish commented 11 years ago

There is just another thing, which I could not yet achieve using glue. For some of our models we use single table inheritance. So each of those models gets its own model file and rabl representation. To not "case-when" over all possible model names we something like this:

# item.json.rabl
object :@item => :item
attributes :common_stuff
node(nil, if: ->(item) { item.class < BaseItem }) do |item|
  partial "/yada-yada/#{item.class.to_s.underscore}", object: item
end

So, to know the path to the partial I need access to the actual instance of that item and the only place where I can get it ist inside a node block.

onyxraven commented 11 years ago

Yeah, this is almost exactly the issue I ran across. (even more, i'm looping over a list of STI objects and creating an array of hashes). I also do a part where I grab an association of the root object and merge another partial using the 'code' block in rabl proper:

code do |o|
  unless o.customer_offer.nil?
    partial('models/customer_offer', object: o.customer_offer)
  end
end

tricky part is I'm currently doing some of these within extends templates to reduce copy-pasta. I know that the combination of these things is why my rabl templates are wicked slow, and I'm likely going to end up going to dealing with the result hashes at a lower level instead of in rabl :-/

ccocchi commented 11 years ago

@onyxraven For your example you can use

condition(->(o) { !o.customer_offer.nil? }) do
  glue(:customer_offer) { partial('models/customer_offer') }
end

@fnordfish Yeah glue was only conceive to merge attributes from association into the parent object, but I will review your PR.

For the STI problem, I dont really like the node(nil) syntax so I'll try to find another way to achieve this. I'll keep you posted.

fnordfish commented 11 years ago

I totally understand your discomfort about the node(nil) syntax. It's really not obvious what happens there, but more of a hack.

ccocchi commented 11 years ago

What do you think about somethin like this (with a new merge keyword)

object :@item
merge { |i| partial("item/#{i.to_s}", object: i) if i.class < BaseItem }

We cannot use node(nil) because if you have more than one, the latest override the rest.

fnordfish commented 11 years ago

Cool, the only thing about it is, that - well - it introduces a new command for the same thing glue already seems to do, right? What do you think about recycling glue?

glue { |i| partial("item/#{i.to_s}", object: i) if i.class < BaseItem }

Or would this be more confusing, because glue and merge will probably be pretty differently implemented and thus have different performance characteristics?

ccocchi commented 11 years ago

So now you have the merge keyword if you want to add informations directly to the current node. It differs from glue, since its block will be evaluated at rendering time whereas with glue its at compiling time.

node(nil) { ... } is an alias to merge to keep compatibility with original Rabl gem.

Changes are at 3f1976a4f572781085a30e386d229428e6317879 and will be available in the next release.