Add "sibling" method to collection (issue #174) #183

Closed
wants to merge 3 commits into from

3 participants

@inkstak

Hi,

Let's take an example.
I need to get the following (JSON) hash to paginate my data.

{  total: 250,
   page: 3,
   total_pages: 25
   data: [
     { id: 1, name: "Foo", ...},
     { id: 2, name: "Bar", ...}
  ]
}

Server-side. I use kaminari and I can now make that :

collection @items => :data

sibling :total_count => :total, :num_pages => :total_pages, :current_page => :page
attributes :id, :name

"Siblings" are methods to which the collection responds.

I guess my pull request is a first approach, and

@nesquena
Owner

This is an interesting idea and I appreciate the pull request. Still trying to decide if I like the syntax. Adding another major type to the API like this is not something I want to do lightly. I can see the use for something like this though that appends nodes to the root level of a response.

@databyte
Collaborator

Is the sibling method is separate from the refactoring of the result method correct?

Personally, I know that each of the to_[format] methods had a few lines of non-DRY code here and there but creating a mega result method with lots of conditionals makes it much harder to read. If anything, I would suggest separating out the sibling patch from the result refactoring and maybe we could approach that another way.

As for controlling the root, I've been doing this:

object false

child(@inspiration => :inspiration) do
  extends("v1/inspirations/show")
end

node(:foo) { @bar }

which gives me something like:

{
  foo: 'whatever bar is'
  inspiration: [ { stuff }, { more_stuff } ]
}
@nesquena
Owner

Yeah that is the approach I have been taking too. Also the result refactoring seemed confusing to me as well. I prefer keeping that logic separate to the formats IMO

@inkstak

Thanks for the feedbacks

I also used the same method as @databyte suggested (#174)
But I felt like I was cheating and that's the reason of this pull request : to have a cleaner alternative.

The major problem about this patch is that the method collection_root_name is called in every to_[format] methods, not one time at one place. It seems to be linked to the xml output.

I tried to make the patch the lighter possible : apply the siblings one time at one place, without rebuilding the whole architecture of the engine. That's the reason of the refactoring.

However I must concede that refactoring is my obsessive compulsive disorder. So ok, I can suggest that in a separate commit/request and can clean up this pull request to make the changes clearer.

Now I guess that an other discussion could be about the DSL method itself.
Maybe we can introduce a more common method to add root nodes :

root_node count => @items.total_count
root_node page => @page
...

Then "siblings" (or any better name) can be a shortcut using the root node method.

@databyte
Collaborator

I think it does make sense to attempt a collection of root nodes without having to use object false as Jbuilder allows for it easily.

We should find a DSL we like and then maybe @blakink can just tweak your implementation and tests for it. I'm not for "sibling" per-se because I think it may get confused with child.

What if it worked similar to child, glue and node.

collection @users do
  node(:total_count) { |m| @user.posts.count }
end

attributes :id, :foo, :bar

So anything going within each object of collection is outside of the block and within the block are the items that get bolted onto collection itself. Thoughts?

@nesquena
Owner

One alternative is basically support a clearer scoping for collection/object (backwards compatible or not?):

collection @users do
  attributes :id, :foo, :bar
end

node :total_pages do
  @users.total_pages
end

node :current_page do 
  @users.current_page
end

would put the node outside the collection at the root. I want to be careful to not introduce many more commands in the api. I think object, collection, child, node, attribute, glue is already a lot for people to understand. I think being able to do as you propose databyte makes sense too. What do you think @blakink

@nesquena
Owner

What do you guys think about that tho. Making collection optionally into a block with its own dsl rules similar to how child works. Is that too odd to allow things outside the collection block being at the root level (if collection doesn't have a block given)? I can see it working both ways, but it could be confusing. These questions is why I haven't tried to tackle this pull request yet. Not sure of the least confusing syntax.

We could also release RABL 0.6.0 breaking changes and basically requires things to be scoped into their objects or collections:

object @post do
  attributes :id, :name
end

looks pretty intuitive. Perhaps the dsl rules should have been wrapped in object/collection blocks all along. Then working outside at the root just becomes:

collection @posts do
  attributes :id, :name
end

node :total_pages do
  @users.total_pages
end
@databyte
Collaborator

I like what you're suggesting there but of course you have the issue of breaking changes.

The best route would be to allow both and throw a deprecation warning in 0.6.0 until a 1.0.0 release that works "the new way".

The logic could be wrapped around object/collection such that if it doesn't have a block, then use the old way - otherwise go the new route. That's simplifying it a bit but going with the new DSL, you can't really use object without a block unless you're calling object false. In the instance you are using object false, maybe the new DSL doesn't require to do that either. Seems silly to say object false instead of just assuming object is set false until told otherwise.

Then the triggers for the old format are using object false or object/collection without a block.

Maybe @radar has an opinion too.

@nesquena
Owner

Exactly the new proposed changes are actually very syntax simplifying in a lot of ways and more consistent with the rest of rabl. No more object => false, easy addition to the root nodes, and just generally easier to grok I think.

Personally I am not afraid to make breaking changes but I agree basically we should be smart and in 0.6.0 issue deprecation warnings. Treat it the preferred way if a block is yielded, otherwise act in the old way (everything is considered in the block). I think we can do this, if we all agree this is a preferred syntax. Curious on your opinion too @achiu.

I am going to open another issue dedicated to this idea.

@databyte databyte closed this Aug 6, 2012
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment