rendering empty collections patch

[Frederick Cheung]

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

[Chris Eppstein]

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

On Aug 13, 1:31 pm, Frederick Cheung <frederick.che...@gmail.com>
wrote:
> Hello all,
>
> I've got a smallish patch athttp://rails.lighthouseapp.com/projects/8994/tickets/791-add-empty_te...

[Duncan Beevers]

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

[Rosen, James A.]

+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
jro...@mitre.org
781.377.0155

[Damian Janowski]

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).

[Chris Eppstein]

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

On Aug 13, 1:46 pm, "Duncan Beevers" <duncanbeev...@gmail.com> wrote:
> 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
>
> On Wed, Aug 13, 2008 at 1:31 PM, Frederick Cheung
>

> <frederick.che...@gmail.com> wrote:
>
> > Hello all,
>
> > I've got a smallish patch athttp://rails.lighthouseapp.com/projects/8994/tickets/791-add-empty_te...

[Frederick Cheung]

On Aug 13, 10:00 pm, "Damian Janowski" <damian.janow...@gmail.com>
wrote:

>
> 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).

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

[Frederick Cheung]

On Aug 13, 9:46 pm, "Duncan Beevers" <duncanbeev...@gmail.com> wrote:
> 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

>
> +1 on an empty result-set option
> -1 on default _empty_result partial
>
> On Wed, Aug 13, 2008 at 1:31 PM, Frederick Cheung
>

> <frederick.che...@gmail.com> wrote:
>
> > Hello all,
>
> > I've got a smallish patch athttp://rails.lighthouseapp.com/projects/8994/tickets/791-add-empty_te...

[Hongli Lai]

On Aug 13, 10:39 pm, Chris Eppstein <chriseppst...@gmail.com> wrote:
> 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.

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.

[Michael Koziarski]

> 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.

--
Cheers

Koz

[Chris Cruft]

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

On Aug 13, 5:00 pm, "Damian Janowski" <damian.janow...@gmail.com>
wrote:

> On Wed, Aug 13, 2008 at 5:31 PM, Frederick Cheung
>
>
>
> <frederick.che...@gmail.com> wrote:
>
> > Hello all,
>

> > I've got a smallish patch athttp://rails.lighthouseapp.com/projects/8994/tickets/791-add-empty_te...

[S. Brent Faulkner]

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

[Ryan Bates]

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

[Damian Janowski]

On Thu, Aug 14, 2008 at 8:48 AM, Chris Cruft <cc...@hapgoods.com> wrote:
> 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.

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') %>

[Josh Susser]

On Aug 14, 2008, at 11:43 AM, Damian Janowski wrote:
> 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.

--
Josh Susser
http://blog.hasmanythrough.com

[S. Brent Faulkner]

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

[Frederick Cheung]

Yeah, I'm sold on the ternary. Not sure why the thought didn't occur
to me before.

Fred

[Chris Cruft]

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

On Aug 14, 4:34 pm, Frederick Cheung <frederick.che...@gmail.com>
wrote:

[Ryan Bates]

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

[Ryan Bates]

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

On Aug 15, 8:07 am, Ryan Bates <r...@railscasts.com> wrote:
> 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/...