Skip to content

Proposed (eventually) breaking object syntax change #216

Open
nesquena opened this Issue Apr 6, 2012 · 44 comments
@nesquena
Owner
nesquena commented Apr 6, 2012

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
Collaborator
databyte commented Apr 6, 2012

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
Owner
nesquena commented Apr 6, 2012

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
Owner
nesquena commented Apr 6, 2012

Any thoughts @mschulkind ?

@databyte
Collaborator
databyte commented Apr 6, 2012

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
Owner
nesquena commented Apr 6, 2012

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
Collaborator
databyte commented Apr 6, 2012

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
niuage commented Apr 6, 2012

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

@nesquena
Owner
nesquena commented Apr 6, 2012

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
inkstak commented Apr 6, 2012

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
Collaborator
databyte commented Apr 6, 2012

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
Owner
nesquena commented Apr 6, 2012

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

@davejamesmiller

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
Owner
nesquena commented Apr 6, 2012

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

@nesquena
Owner
nesquena commented Apr 6, 2012

@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
Collaborator
databyte commented Apr 6, 2012

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
Owner
nesquena commented Apr 6, 2012

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
Collaborator
databyte commented Apr 6, 2012

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
Owner
nesquena commented Apr 6, 2012

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
radar commented Apr 7, 2012

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
mackuba commented Apr 8, 2012

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
svoisen commented Apr 10, 2012

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

@ph
ph commented Apr 10, 2012

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

@nesquena
Owner

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
tilo commented Apr 25, 2012

+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
Collaborator

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
Owner

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
niuage commented Sep 5, 2012

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

@databyte
Collaborator
databyte commented Sep 5, 2012

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

@niuage
niuage commented Sep 6, 2012

Ok, good. I was worried because @nesquena got started really quickly, but then, nothing happened for 5 months (well, at least on this branch) ;) I was really excited by this change ^^.

@nesquena
Owner
nesquena commented Sep 6, 2012

This is not abandoned at all, this is the future of RABL (towards a 1.0), it's a work in progress. I want to get it right.

@Tonkpils
Tonkpils commented Dec 3, 2012

To get these features of blocks for objects and collections I must use the object_syntax_change branch right? I'm running into the problem of having to specify nodes at the root level and its getting a bit hectic.

@nesquena
Owner
nesquena commented Dec 3, 2012

I wouldn't use the branch just yet, it's not really baked. If you want to post an issue, happy to take a look at if there's a way to clean up what you need to do. If it's a bunch of root level nodes that are not tied to a particular model though, it may well not be a use case with a clean fit to RABL.

@Tonkpils
Tonkpils commented Dec 3, 2012

Nevermind, I happened to fix the issue. it is not so much that I have a bunch of root level nodes, but just to include a status/message root level node I need to set object false and code didn't look as clean as it could be. But I hope the gem gets updated soon! :+1:

@nesquena
Owner
nesquena commented Dec 3, 2012

Ok, hopefully it didn't make it too unsightly, i.e:

object false

child @foo do 
  # ...
end

node :status do
  200
end

or you could use a layout

@nesquena
Owner

@databyte OK I want to jump back into this now and this time let's close it down. Let's plan a time to work on this together soon.

@DouweM
Collaborator
DouweM commented Jul 12, 2013

@nesquena And... this issue and the branch in question have been dead for 6 months. Any plans on reviving this? :smile:

@nesquena
Owner

Working on too many projects at the moment, and rabl has been serving my own projects and apps for a while happily. Always hope to get back to this at some point, I do want these changes to happen.

@DouweM
Collaborator
DouweM commented Jul 12, 2013

Well, it's good to hear that you haven't forgotten about it. I would look into it myself, but I'm also too busy right now to do anything other than fix bugs along the way.

@shyam-habarakada

I would love to see this, assuming it addresses performance issues like #390 that @chancancode opened and @databyte closed against this. I've been using RABL for almost two years now on a couple of projects as well, and with the size and complexity of my data growing, performance is starting to become more of an issue than the actual syntax.

In the meantime, is there an updated suggestion on #390 ?

Also, the performance issue #260

Thanks

@nesquena
Owner

Hi @shyam-habarakada, thanks for inquiring. Appreciate you've been using rabl successfully and I'm sorry to hear about the performance issues being a real problem. Part of the issue here is one of a lot of open-source and that is rabl in it's current form works perfectly for my purposes which are simple light-weight sinatra APIs (using RABL templates). I don't use Rails right now day-to-day and I don't experience a lot of performance concerns because I'm working with paginated data sets and I never see RABL taking very long to render (in fact its so fast for my purposes, it's a complete non-issue).

I've been shifting more of my focus to other projects since RABL essentially solves my use case very well. That said, I am more than happy to continue maintaining this project and fixing any major bugs that arise, merging in patches, etc. If there's ever cases where RABL breaks down for my sweet spot, I'd probably fix it. I'd love for others who are feeling the pain to fix these issues though if they are interested in different pain points. If anyone sends me a pull request or two that I can merge in, I'd happily give anyone commit access so they can help fix RABL in these areas.

@shyam-habarakada
@nesquena
Owner

I think a page on the wiki https://github.com/nesquena/rabl/wiki for that would be great although I'm not aware of any page on there at the moment that focuses on how to do performance profiling. Closest is this page discussing various tips for using RABL in production: https://github.com/nesquena/rabl/wiki/Rabl-In-Production

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.