I''ve got a couple of tickets filed for simply_helpful dealing with render(:partial) ([1], [2]); does anybody have any thoughts? Am I misunderstanding the simply_helpful code, or are these real? It''d be nice to see these get checked in (or at least reviewed) if they are actual bugs. Nick [1] Ticket 6719: http://dev.rubyonrails.org/ticket/6719 [2] Ticket 8410: http://dev.rubyonrails.org/ticket/8410 --~--~---------~--~----~------------~-------~--~----~ You received this message because you are subscribed to the Google Groups "Ruby on Rails: Core" group. To post to this group, send email to rubyonrails-core@googlegroups.com To unsubscribe from this group, send email to rubyonrails-core-unsubscribe@googlegroups.com For more options, visit this group at http://groups.google.com/group/rubyonrails-core?hl=en -~----------~----~----~----~------~----~------~--~---
> It''d be nice to see these get checked in (or at least reviewed) if > they are actual bugs.Hi Nick, Simply Helpful has been folded into core. Could you check if these problems are still present on edge? If so, please do post again here and I''ll apply. --~--~---------~--~----~------------~-------~--~----~ You received this message because you are subscribed to the Google Groups "Ruby on Rails: Core" group. To post to this group, send email to rubyonrails-core@googlegroups.com To unsubscribe from this group, send email to rubyonrails-core-unsubscribe@googlegroups.com For more options, visit this group at http://groups.google.com/group/rubyonrails-core?hl=en -~----------~----~----~----~------~----~------~--~---
Doh! When did that happen? Last time I checked it was still waiting to
be folded.
Yes, the core seems to have the same problems. I''ll refile the bugs as
soon as Trac is back up (I''m getting a ''duplicate key violates
unique
constraint "node_change_rev_key" '' error).
For now, here are the problems I see (this is all assuming you''re
using record identification):
For reference, here''s how the variables are passed:
in action_view/base.rb, line 308:
render_partial is called with the object, wrapped nil, locals
which means that in partials.rb, line 50:
partial_path is the object, local_assigns is wrapped nil,
deprecated_local_assigns is the locals hash
Here are the problems:
1. when the object is an Array (i.e., we''re rendering a collection)
(line 67):
we sensibly name path and collection
we call render_partial_collection with the correct path and
collection, but we use local_assigns.value, which is nil (see above).
Thus locals are not distributed over collections when using
identification.
2. when the object is not an array (i.e., we''re rendering a single
object) (line 75):
we call render_partial with the path, local_assigns as the object
(which is a wrapped nil), and deprecated_local_assigns as the locals
hash (which is the actual locals hash)
Thus the object is not properly forwarded to
render_partial(string ...).
Also, for the collection, the test for a trivial collection is
inconsistent with the assumptions made: if we test for any?, then we
should use the first non-nil value, instead of just the first.
Here''s a suggested patch:
http://pastie.caboo.se/63352
Nick
On May 19, 5:10 pm, DHH <david.heineme...@gmail.com>
wrote:> > It''d be nice to see these get checked in (or at least
reviewed) if
> > they are actual bugs.
>
> Hi Nick,
>
> Simply Helpful has been folded into core. Could you check if these
> problems are still present on edge? If so, please do post again here
> and I''ll apply.
--~--~---------~--~----~------------~-------~--~----~
You received this message because you are subscribed to the Google Groups
"Ruby on Rails: Core" group.
To post to this group, send email to rubyonrails-core@googlegroups.com
To unsubscribe from this group, send email to
rubyonrails-core-unsubscribe@googlegroups.com
For more options, visit this group at
http://groups.google.com/group/rubyonrails-core?hl=en
-~----------~----~----~----~------~----~------~--~---
ObjectWrapper, not WrapperObject. http://pastie.caboo.se/63368 On May 21, 2:32 pm, Nick Howell <nlhow...@gmail.com> wrote:> Doh! When did that happen? Last time I checked it was still waiting to > be folded. > > Yes, the core seems to have the same problems. I''ll refile the bugs as > soon as Trac is back up (I''m getting a ''duplicate key violates unique > constraint "node_change_rev_key" '' error). > > For now, here are the problems I see (this is all assuming you''re > using record identification): > > For reference, here''s how the variables are passed: > > in action_view/base.rb, line 308: > render_partial is called with the object, wrapped nil, locals > > which means that in partials.rb, line 50: > partial_path is the object, local_assigns is wrapped nil, > deprecated_local_assigns is the locals hash > > Here are the problems: > 1. when the object is an Array (i.e., we''re rendering a collection) > (line 67): > we sensibly name path and collection > we call render_partial_collection with the correct path and > collection, but we use local_assigns.value, which is nil (see above). > > Thus locals are not distributed over collections when using > identification. > > 2. when the object is not an array (i.e., we''re rendering a single > object) (line 75): > we call render_partial with the path, local_assigns as the object > (which is a wrapped nil), and deprecated_local_assigns as the locals > hash (which is the actual locals hash) > > Thus the object is not properly forwarded to > render_partial(string ...). > > Also, for the collection, the test for a trivial collection is > inconsistent with the assumptions made: if we test for any?, then we > should use the first non-nil value, instead of just the first. > > Here''s a suggested patch: > > http://pastie.caboo.se/63352 > > Nick > > On May 19, 5:10 pm, DHH <david.heineme...@gmail.com> wrote: > > > > It''d be nice to see these get checked in (or at least reviewed) if > > > they are actual bugs. > > > Hi Nick, > > > Simply Helpful has been folded into core. Could you check if these > > problems are still present on edge? If so, please do post again here > > and I''ll apply.--~--~---------~--~----~------------~-------~--~----~ You received this message because you are subscribed to the Google Groups "Ruby on Rails: Core" group. To post to this group, send email to rubyonrails-core@googlegroups.com To unsubscribe from this group, send email to rubyonrails-core-unsubscribe@googlegroups.com For more options, visit this group at http://groups.google.com/group/rubyonrails-core?hl=en -~----------~----~----~----~------~----~------~--~---
Trac seems to be back up; ticket 8426 ( http://dev.rubyonrails.org/ticket/8426 ). Nick On May 21, 3:41 pm, Nick Howell <nlhow...@gmail.com> wrote:> ObjectWrapper, not WrapperObject. > > http://pastie.caboo.se/63368 > > On May 21, 2:32 pm, Nick Howell <nlhow...@gmail.com> wrote: > > > Doh! When did that happen? Last time I checked it was still waiting to > > be folded. > > > Yes, the core seems to have the same problems. I''ll refile the bugs as > > soon as Trac is back up (I''m getting a ''duplicate key violates unique > > constraint "node_change_rev_key" '' error). > > > For now, here are the problems I see (this is all assuming you''re > > using record identification): > > > For reference, here''s how the variables are passed: > > > in action_view/base.rb, line 308: > > render_partial is called with the object, wrapped nil, locals > > > which means that in partials.rb, line 50: > > partial_path is the object, local_assigns is wrapped nil, > > deprecated_local_assigns is the locals hash > > > Here are the problems: > > 1. when the object is an Array (i.e., we''re rendering a collection) > > (line 67): > > we sensibly name path and collection > > we call render_partial_collection with the correct path and > > collection, but we use local_assigns.value, which is nil (see above). > > > Thus locals are not distributed over collections when using > > identification. > > > 2. when the object is not an array (i.e., we''re rendering a single > > object) (line 75): > > we call render_partial with the path, local_assigns as the object > > (which is a wrapped nil), and deprecated_local_assigns as the locals > > hash (which is the actual locals hash) > > > Thus the object is not properly forwarded to > > render_partial(string ...). > > > Also, for the collection, the test for a trivial collection is > > inconsistent with the assumptions made: if we test for any?, then we > > should use the first non-nil value, instead of just the first. > > > Here''s a suggested patch: > > >http://pastie.caboo.se/63352 > > > Nick > > > On May 19, 5:10 pm, DHH <david.heineme...@gmail.com> wrote: > > > > > It''d be nice to see these get checked in (or at least reviewed) if > > > > they are actual bugs. > > > > Hi Nick, > > > > Simply Helpful has been folded into core. Could you check if these > > > problems are still present on edge? If so, please do post again here > > > and I''ll apply.--~--~---------~--~----~------------~-------~--~----~ You received this message because you are subscribed to the Google Groups "Ruby on Rails: Core" group. To post to this group, send email to rubyonrails-core@googlegroups.com To unsubscribe from this group, send email to rubyonrails-core-unsubscribe@googlegroups.com For more options, visit this group at http://groups.google.com/group/rubyonrails-core?hl=en -~----------~----~----~----~------~----~------~--~---
> Trac seems to be back up; ticket 8426 (http://dev.rubyonrails.org/ticket/8426Good stuff. Could you also add some tests that verify the behavior? We''re trying to enforce that on all new patches. Thanks! --~--~---------~--~----~------------~-------~--~----~ You received this message because you are subscribed to the Google Groups "Ruby on Rails: Core" group. To post to this group, send email to rubyonrails-core@googlegroups.com To unsubscribe from this group, send email to rubyonrails-core-unsubscribe@googlegroups.com For more options, visit this group at http://groups.google.com/group/rubyonrails-core?hl=en -~----------~----~----~----~------~----~------~--~---
Not sure if my last message made it through, so I''m reposting. I came up with tests; one of them still fails, though: the collection- containing-nil one. I''m not really sure what to do with that; anyone have any thoughts on what should happen if you try to render :partial => nil ? What about render :partial => [nil, a_model] ? My tests expect that render :partial => [nil, a_model] is the same as render :partial => a_model Nick On May 21, 9:24 pm, DHH <david.heineme...@gmail.com> wrote:> > Trac seems to be back up; ticket 8426 (http://dev.rubyonrails.org/ticket/8426 > > Good stuff. Could you also add some tests that verify the behavior? > We''re trying to enforce that on all new patches. Thanks!--~--~---------~--~----~------------~-------~--~----~ You received this message because you are subscribed to the Google Groups "Ruby on Rails: Core" group. To post to this group, send email to rubyonrails-core@googlegroups.com To unsubscribe from this group, send email to rubyonrails-core-unsubscribe@googlegroups.com For more options, visit this group at http://groups.google.com/group/rubyonrails-core?hl=en -~----------~----~----~----~------~----~------~--~---
> I came up with tests; one of them still fails, though: the collection- > containing-nil one. I''m not really sure what to do with that; anyone > have any thoughts on what should happen if you try to render :partial > => nil ? What about render :partial => [nil, a_model] ? My tests > expect that render :partial => [nil, a_model] is the same as > render :partial => a_modelI like just ignoring nil. As in, let it call .compact on the collection before starting to work with it. Be sure to add this to the docs, though. --~--~---------~--~----~------------~-------~--~----~ You received this message because you are subscribed to the Google Groups "Ruby on Rails: Core" group. To post to this group, send email to rubyonrails-core@googlegroups.com To unsubscribe from this group, send email to rubyonrails-core-unsubscribe@googlegroups.com For more options, visit this group at http://groups.google.com/group/rubyonrails-core?hl=en -~----------~----~----~----~------~----~------~--~---
Sounds good; I''m uploading a new patch. All the tests I wrote pass now, and I added a doc note. One last thing: I''m wondering if we should add a case for AssociationCollections, so that you can have class CatOwner has_many :cats end render(:partial => @cat_owner.cats) It might look like when ActiveRecord::Associations::AssociationCollection render_partial(partial_path.to_a, local_assigns, deprecated_local_assigns) This doesn''t happen automagically, I don''t think... Thoughts? Nick On May 25, 9:27 am, DHH <david.heineme...@gmail.com> wrote:> > I came up with tests; one of them still fails, though: the collection- > > containing-nil one. I''m not really sure what to do with that; anyone > > have any thoughts on what should happen if you try to render :partial > > => nil ? What about render :partial => [nil, a_model] ? My tests > > expect that render :partial => [nil, a_model] is the same as > > render :partial => a_model > > I like just ignoring nil. As in, let it call .compact on the > collection before starting to work with it. Be sure to add this to the > docs, though.--~--~---------~--~----~------------~-------~--~----~ You received this message because you are subscribed to the Google Groups "Ruby on Rails: Core" group. To post to this group, send email to rubyonrails-core@googlegroups.com To unsubscribe from this group, send email to rubyonrails-core-unsubscribe@googlegroups.com For more options, visit this group at http://groups.google.com/group/rubyonrails-core?hl=en -~----------~----~----~----~------~----~------~--~---
> One last thing: I''m wondering if we should add a case for > AssociationCollections, so that you can have > > class CatOwner > has_many :cats > end > > render(:partial => @cat_owner.cats)That already works. When we get an array, we just call render :partial for each of the members and join it. Don''t want AP to know anything explicitly about AR. We could be collecting ARes or other model objects and it should work just the same. --~--~---------~--~----~------------~-------~--~----~ You received this message because you are subscribed to the Google Groups "Ruby on Rails: Core" group. To post to this group, send email to rubyonrails-core@googlegroups.com To unsubscribe from this group, send email to rubyonrails-core-unsubscribe@googlegroups.com For more options, visit this group at http://groups.google.com/group/rubyonrails-core?hl=en -~----------~----~----~----~------~----~------~--~---