Hello all,
I've got a smallish patch at http://rails.lighthouseapp.com/projects/8994/tickets/791-add-empty_template-option-when-rendering-a-collection that i'd be interested in feedback on
in a nutshell <% if results.any? %> <%= render :partial => 'result', :collection => results %> <% else %> <em> Nothing found </em> <% end %> BAD
<%= render :partial => 'result', :collection => results, :empty => '<em>Nothing Found</em>' %>
GOOD
Fred
A more rails-y approach would be to have a convention over an explicit behavior. E.g. if a partial named _empty_result was found then it would be called instead if the collection was empty. Of course there's backwards compatibility issues and so you'll probably get the typical "not in core... make a plugin" which is fine, but it's such a small thing, I'd never install a plugin for this behavior.
+0.5
chris
I think it would be more appropriate to supply the name of a template to render when no results are found, similar to how :spacer_template is provided.
<% render :partial => 'result', :collection => 'results', :empty_template => 'shared/no_results' %> or some such. Locals passed explicitly to the collection partial should probably also be provided to the empty_template partial.
+1 on an empty result-set option -1 on default _empty_result partial
+1 on :empty_set option
Undecided on the default _empty_... partial.
What about the related :nil option for non-collection objects that don't exist?
-James Rosen jrosen@mitre.org 781.377.0155
I think render should return nil, so that you can do something even clearer to the eye:
<%= render(:partial => @results) || "Nothing found." %>
(This is backwards compatible too, as ERb will call #to_s on nil).
I like the idea of :empty_template overrides are good. But what is the reason to not have a convention around the value of :empty_template?
chris
The current implementation returns ' ' (ie a single space) on an empty collection and the tests assert this, so it would not quite be backwards compatible (I'm not sure why this happens. I imagine it's for the same reason that render :nothing renders a single space).
I've got helpers which render one or more partials and concatenate them, which would be broken by this change.
Fred
I think it would be more appropriate to supply the name of a template to render when no results are found, similar to how :spacer_template is provided.
<% render :partial => 'result', :collection => 'results', :empty_template => 'shared/no_results' %> or some such. Locals passed explicitly to the collection partial should probably also be provided to the empty_template partial.
Are locals passed to space_template ? passing the locals though is probably a good idea, although the current implementation should allow you to do render ..., :empty => {:partial => 'foo', :locals => {...}}
The current patch allows for either templates or just a string (unlike the original patch - sorry to those who just read the ticket description) mainly because on balance it felt a bit heavyweight to have to create a template for something like this.
Fred
I don't think this is a good idea. It would certainly break current applications that expect render :collection to do nothing if the collection is empty. Futhermore, I often render partials that belong to other controllers (e.g. "render :partial => 'messages/message'" in users/show.html.erb); I wouldn't want Rails to look for an empty_result partial in the current controller's view folder.
I don't think this is a good idea. It would certainly break current applications that expect render :collection to do nothing if the collection is empty. Futhermore, I often render partials that belong to other controllers (e.g. "render :partial => 'messages/message'" in users/show.html.erb); I wouldn't want Rails to look for an empty_result partial in the current controller's view folder.
Yeah, I don't like the idea either, adding a bunch of options to render to cater for this just seems counter intuitive. Typically it's not this simple either, if @items is empty I probably don't want to render the <table> or </table> elements which the _item partial assumes is there.
I like Damian's solution of returning nil. True, it's not entirely backwards compatible, but the real-world impact might be negligible.
Incidentally, to address Frederick's implied question... I don't think the justification for rendering a single space with an empty collection is driven by the same rationale as rendering a single space with render(:nothing => true). Based on the docs, IIRC, a completely empty body causes some browsers (Safari?) to get unhappy. That's not likely to be the result of an empty partial -but it is the result of render :nothing.
So I'm stumped as to the logic behind returning a space on rendering an empty collection. It doesn't resolve the empty body problem above, nor does it solve the HTML compliance problem of having a list with no elements. So unless someone can come up with a good justification for the behavior, I would prefer a change to the signature of render and return nil.
-Chris
I like the general idea of...
render :partial => 'result', :collection => @records, :empty_template => 'shared/no_results'
It seems consistent with other behaviours as noted (such as :spacer_template).
A possible alternative that would extend to other forms of render calls could be to add a render_if method (and a render_unless method) which works similar to link_to_if (taking an optional block). For example...
render_if @post.comments.any?, :partial => 'comment', :collection => @post.comments do content_tag 'em', 'no comments' end
...or...
render_unless @post.locked?, :partial => 'new_comment'
...or...
render_if @result.win?, :partial => 'prize' do render :partial => 'consolation' end
If no block is provided the behaviour could be either to render nothing (or, if someone insists, to remain consistent with other renders and output a single space).
Cheers.
--Brent
I don't think using a block is a good idea for this. Blocks are already used for "render :template".
I'm not convinced the problem needs fixing at all. Is the "if" condition really that ugly? Moving a "No comments" one liner into a partial seems worse to me, and putting HTML into a string and passing it to the render call which already has a bunch of options doesn't appeal to me either. As far as readability is concerned, I prefer the explicit if condition.
Regards,
Ryan
Yeah, I'm sure it's because of Safari not reading your response if you don't give it at least a space character.
Returning nil is a tiny breakage (it only breaks the case where you blindly trust a method to return a String and call + on that. You should be calling to_s on what you get anyways.)
I'm -1 on adding the conditional logic to render. I like this even better:
<%= @posts.blank? ? render(:partials => @posts) : "Nothing found." %>
or
<%= render(:partial => @posts.blank? ? @posts : 'empty_partial') %>
Rendering nil should probably be the same as rendering an empty collection. I'm currently chasing a bug where render :partial with :collection => nil blows up, though I'm having trouble reproducing it reliably. Sounds like this change might sidestep that bug though.
yes, didn't think of that block case... I didn't like the embedded HTML string and was trying to solve that problem instead of the original.
agreed.
-1 to the whole idea... Damian's ternary operator approach is the nice simple solution to the whole deal
Yeah, I'm sold on the ternary. Not sure why the thought didn't occur to me before.
Fred
Sounds like this is heading towards a "less is more" approach, which is so very Railsy.
But why not take it to the logical conclusion: rendering an empty collection should return nil. A space, frankly, sounds like a bug. Then you could use the ternary operator or the classic short-circuit "or".
Of course, I won't be doing the work or explaining the tiny backwards incompatibility...
-Chris
I don't consider the space a bug. It is intentional as there is a test for it:
http://github.com/rails/rails/tree/master/actionpack/test/controller/new_render_test.rb#L810
As some have concluded above, this is because Safari doesn't like an empty response. There are times when all you're returning is "render :collection", say for example, on a javascript request.
However, if this returns nil instead of a space, I think it's possible to translate this into a space higher up in the response chain. This way the "render" method itself doesn't have to worry about returning a space.
Regards,
Ryan
I posted a patch at this ticket which moves the " " logic later on in the response chain so render collection returns nil given an empty collection.
Please try it out and give feedback.
Regards,
Ryan