rendering empty collections patch

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.

http://rails.lighthouseapp.com/projects/8994-ruby-on-rails/tickets/791-add-empty_template-option-when-rendering-a-collection#ticket-791-6

Please try it out and give feedback.

Regards,

Ryan