Proposed (eventually) breaking object syntax change

[nesquena]

trafficstars

Issue #183 started an interested discussion between me and @databyte and @blakink. What if we changed the syntax to group the content into object/collection blocks. For instance, take this example from today:

collection @posts => :posts

attributes :id, :name, :body, :created_at

node :is_read do |p|
  p.read_by? @user
end

child :author do |p|
  attributes :name, :username, :email
end

Now consider this syntax change where the rules are logically grouped into the collection with a block:

collection :posts => @posts do |p|
  attributes :id, :name, :body, :created_at

  node :is_read do |p|
    p.read_by? @user
  end

  child :author do |p|
    attributes :name, :username, :email
  end
end

I find the second way more consistent and intuitive IMO. I think we could potentially deprecate in 0.6.0 and remove it in 0.7.0. This is a beta and I am not afraid to be willing to switch syntax if we can agree that it's a step in the right direction.

It also makes adding root nodes easier or even multiple objects:

collection :posts => @posts do |p|
  # ...
end

node :total_pages do
   @posts.total_pages
end

node :current_page do
  @posts.current_page
end

as well as accessing the object in a consistent way:

collection :posts => @posts do |p|
  if p.author.admin?
     # ... group based on the post in the collection
  end
end

Summary

In short, here's what we want to do:

Change object syntax into a block

object @post do |p|
  attributes :id, :name, :body, :created_at
end

Change collection syntax into a block

collection :posts => @posts do |p|
  attributes :id, :name, :body, :created_at

  node :is_read do |p|
    p.read_by? @user
  end

  child :author do |p|
    attributes :name, :username, :email
  end
end

Reverse hash syntax from

collection @posts => :posts

to

collection :posts => @posts

Stricter handling of collection and object.

If the developer says 'object' then assume object, if they say collection then assume collection. We no longer will detect this ourselves.

[databyte]

Maybe we can tackle Issue 119 as well by not having to specify:

collection @posts => :posts do |p|
  # ...
end

and instead allow

collection @posts do |p|
  # ...
end

with handling an empty/nil @posts properly?

Also I would bump up to v1.0, see point 9 of semver.org.

[nesquena]

Yeah I think that makes sense. And point taken we can do this with a 1.0 release (after maybe an rc or 2)

[nesquena]

Any thoughts @mschulkind ?

[databyte]

Can I close Pull #178 as well? Anything we need to mention here to make sure that happens as well? (I don't like seeing projects with lots of open issues so I'm going to comment and close what I can.)

[nesquena]

I would leave #178 unless you don't think we need that option. Do you think these proposed changes would affect the need for that as well? They seem possibly complementary changes. We could also close that ticket and just make a note to include that functionality in our proposed 1.0 release.

[mschulkind]

I'd love a change like this. It would mean I don't need my code(nil) patch on my branch anymore. The cleanup is great as well. This would make things far more clear.

[databyte]

I think bringing several changes into a 1.0 milestone makes the most sense. That way anyone with forks or hacks can address those once throughout all their templates vs piecemeal as we make incremental changes. Like what I ended up doing for FactoryGirl 3.0, I just sat down for an hour and dealt with all the changes at once.

[niuage]

I love this (proposed) change. Especially because it makes working with multiple objects easy and clear.

[nesquena]

I agree, I think in general this new syntax "feels" more intuitive and consistent. I will see if I can spike on it in a new branch soon.

[inkstak]

I also love the new syntax and this way of scoping. Really much more intuitive and logic. You'll be able to close the issue #174.

One last question... how to handle the following case ?

# Initializers

Rabl.configure do |config|
  config.include_json_root = false
end

# Template

collection @posts do |p|
  # ...
end

node :total_pages do
   @posts.total_pages
end

In this case, the collection will output an array, not a hash. So, should we add a default collection key if more than 1 node is defined ? The output could look like :

 { collection: [....], total_pages: xx }

[databyte]

Yeah, most likely the need to add other nodes would force create that root node. Instead of a default collection key, it could revert back to using posts. The logic being, only exclude the root node if only a collection/object block is used and include_json_root is false.

[nesquena]

Yeah that would make sense to me too, just add back the root node if there's nodes outside the objects array.

[d13r]

While you're discussing syntax changes, I have never understood why RABL uses:

collection @posts => :posts

Rather than:

collection :posts => @posts

It seems to be inconsistent with everything else I've seen in Ruby/Rails, where the key is a generally a symbol and the value is the complex object.

Also, in Ruby 1.9 this second form can be simplified to:

collection posts: @posts

Which matches nicely with the JSON syntax that it outputs:

{ posts: [...] }

The same applies to the other methods (object, attributes, etc.)

[nesquena]

Fair point maybe we can rectify all of these things as part of one big breaking release

[nesquena]

@databyte First very rough stab of this proposed change (nested objects and collections) now lives here: https://github.com/nesquena/rabl/tree/object_syntax_change I updated all the tests to use the basics of the new syntax (with support for the old syntax as well).

It's rough right now but all the existing tests pass except one which is related to rails 3.2 caching and I added a FIXME to. There's still a lot to do but if you (and anyone else who reads this) could take a look, would love any help and/or feedback towards achieving this better syntax.

[databyte]

Yeah, so the problem is that when the cache keyword is called within an extended template, the object is not set yet.

fetching rails source posts/show
method cache called - key is , object_data is #<Object:0x007fb2d729b648>
method object called - data is set to #<Post:0x007fb2d41cabe0>
method object called - data is set to 2012-04-06 17:05:08 UTC

So this is after posts/index is being processed and it reaches the partial posts/show for inclusion. It hit the cache method where key is missing and the current object_data is your generic Object.new. Then data is set to an actual Post object. Normally cache would be on the next line and it'd work since it's set next.

Maybe an alternative is to set the data object when within a collection without having to use the object method? So that anytime you iterate over a collection, it'll immediately set object making the use of object within extended template even optional in that case.

[nesquena]

I see, thank for digging in. I think getting this new syntax all working right is going to be a bit of effort but I think it will be a worthwhile improvement to the project. When you get a chance, can you take a look at how I am structuring object_node and refactoring engine and see if it makes sense, if you have any suggestions.

[databyte]

Sure thing, I'll look at it. I didn't expect you to spike it out so quickly. Another thing we could look into is moving cache key and options into an argument of collection/object. That way you could do something like this:

object @post do...

object @post, { cache: @post } do...

object foo: @post, { cache: @post } do...

object @post, { cache: ['kittens!', @post], cache_options: { expires_in: 1.hour }  } do...

or have it be a block too like it is in Rails.

object @post do
  cache @post do
    attributes :id, :name
  end
end

Which I personally don't prefer but simply mention it because it's flexible in this sense:

object @post do
  cache @post do
    attributes :id, :name
  end

  attributes :stuff_i_do_not_cache
end

[nesquena]

Yeah I could see caching becoming object/collection-centric. Not sure what makes the most sense from that sense. I could also see it being object-centric with:

object @post do
   cache @post 
   # or cache true
   attributes :foo, :bar
end

[radar]

I don't mind the changes discussed in this thread. Really, it's up to you guys. Whatever you change, we'll adapt to just fine.

[mackuba]

Totally +1. I've tried using Rabl in my current project, but so far I'm not convinced because it's sometimes a bit unintuitive, and some non-standard structures aren't easy to define. I think these changes would help a lot with that.

[svoisen]

+1 for the proposed changes. Seems much more intuitive and more in line with the Ruby/Rails way of doing things.

[ph]

+1 with the changes it look a lot more idiomatic.

[nesquena]

Glad you guys are on board with the changes, hope to get a workable version with these proposed changes in soon.

[sjmadsen]

I'm also on board with these changes. Anything that is more in line with common 1.9 syntax is a good thing.

I ran across this today after bumping up against issue #119, where my JSON included an empty string as a key with an empty array as its value ({"":[]}). The client code didn't like that one bit. The workaround discussed in other issues fixes it for now, but the syntax is not ideal.

[tilo]

+1 for the proposed changes, including the :posts => @posts

more and updated examples and documentation would be great - especially for cases where ppl have problems with

[databyte]

As mentioned in Issue #277, if you use object or collection, the engine should build it out accordingly without attempting to detect if the "object" is enumerable-like. Can we drop the use of determine_object_root and the helpers is_object? and is_collection? with it. Or maybe only use it with child and not with the object/collection?

How would this work then?

# user/show.rabl

object @user do      # developer says it's only an object - don't check enumeration
  attributes :name, email: :email_address

  child :spouse do   # object - it's not enumerable so don't iterate
    attributes :age, :happiness_level
  end

  child :kids do     # collection - check it's enumerable before iterating
    attributes :age, :school, :grade_level
  end
end

collection @cars do  # developer says it's a collection - throw exception if otherwise
  attributes :make, :model

  node :pimp_mobile do |car|
    car.style == :minivan
  end
end

I also think we could start aliasing some of the keywords and go with a smaller keyword set such that partial and extends do more or less the same thing while glue is probably not needed once you start using blocks.

How's this weekend looking? :grin:

[nesquena]

Yeah I could definitely see that. We could use it for child and avoid it for object/collection as part of these changes. Good suggestion. I agree we could probably remove glue too. I think this weekend could work well.

[niuage]

This branch has not been abandoned right? Is it usable yet?

[databyte]

Not abandoned but not usable ... patience before time and time before code.