Always treat the object passed to content_tag_for as an array. #4341

Closed
wants to merge 2 commits into
from

Conversation

Projects
None yet
7 participants
@coop
Contributor

coop commented Jan 6, 2012

Currently content_tag_for checks if the object responds to to_ary to decide what to do with it. This patch eliminates the need to check by wrapping non-array like objects in an array and always iterating over the collection.

@sikachu

This comment has been minimized.

Show comment Hide comment
@sikachu

sikachu Jan 6, 2012

Member

@josevalim what do you think? I think we can also lose content_tag_for_single_record in this case?

Member

sikachu commented Jan 6, 2012

@josevalim what do you think? I think we can also lose content_tag_for_single_record in this case?

@rafaelfranca

This comment has been minimized.

Show comment Hide comment
@rafaelfranca

rafaelfranca Jan 6, 2012

Owner

Internally Array.wrap check if the object responds to to_ary. I agree that this change make the method cleaner, but I think that now the method will be slower.

The most used case is with only one record, so now we had a unnecessaries calls to capture, join and html_safe even with one record.

Owner

rafaelfranca commented Jan 6, 2012

Internally Array.wrap check if the object responds to to_ary. I agree that this change make the method cleaner, but I think that now the method will be slower.

The most used case is with only one record, so now we had a unnecessaries calls to capture, join and html_safe even with one record.

@sikachu

This comment has been minimized.

Show comment Hide comment
@sikachu

sikachu Jan 6, 2012

Member

@rafaelfranca good call.

Member

sikachu commented Jan 6, 2012

@rafaelfranca good call.

@coop

This comment has been minimized.

Show comment Hide comment
@coop

coop Jan 6, 2012

Contributor

Is it worth benchmarking to find out the performance hit to see if it's acceptable or not?

Contributor

coop commented Jan 6, 2012

Is it worth benchmarking to find out the performance hit to see if it's acceptable or not?

@evilmarty

This comment has been minimized.

Show comment Hide comment
@evilmarty

evilmarty Jan 6, 2012

👍

👍

@coop

This comment has been minimized.

Show comment Hide comment
@coop

coop Jan 6, 2012

Contributor

I can remove the call to capture and the tests still pass. I am assuming that that isn't desired behaviour.

Contributor

coop commented Jan 6, 2012

I can remove the call to capture and the tests still pass. I am assuming that that isn't desired behaviour.

@josevalim

This comment has been minimized.

Show comment Hide comment
@josevalim

josevalim Jan 6, 2012

Contributor

Seems good. And yes, we probably don't need the call to capture, unless I am missing something. @sikachu ?

Contributor

josevalim commented Jan 6, 2012

Seems good. And yes, we probably don't need the call to capture, unless I am missing something. @sikachu ?

@sikachu

This comment has been minimized.

Show comment Hide comment
@sikachu

sikachu Jan 6, 2012

Member

I'm not sure. I think that was required in case you have something like render :partial so the content will be captured.

Member

sikachu commented Jan 6, 2012

I'm not sure. I think that was required in case you have something like render :partial so the content will be captured.

@josevalim

This comment has been minimized.

Show comment Hide comment
@josevalim

josevalim Jan 6, 2012

Contributor

The block given to content_tag_for_single_record should already be properly captured. That said, I am fine with merging this patch as long as we get rid of the capture call. Just the extras join and html_safe should not really affect performance.

Contributor

josevalim commented Jan 6, 2012

The block given to content_tag_for_single_record should already be properly captured. That said, I am fine with merging this patch as long as we get rid of the capture call. Just the extras join and html_safe should not really affect performance.

- else
- content_tag_for_single_record(tag_name, single_or_multiple_records, prefix, options, &block)
- end
+ Array.wrap(single_or_multiple_records).map do |single_record|

This comment has been minimized.

Show comment Hide comment
@tenderlove

tenderlove Jan 6, 2012

Owner

Using Array() will also call to_ary:

irb(main):001:0> class Foo; def to_ary; [1,2]; end
irb(main):002:1> end
=> nil
irb(main):003:0> Array(Foo.new)
=> [1, 2]
irb(main):004:0>
@tenderlove

tenderlove Jan 6, 2012

Owner

Using Array() will also call to_ary:

irb(main):001:0> class Foo; def to_ary; [1,2]; end
irb(main):002:1> end
=> nil
irb(main):003:0> Array(Foo.new)
=> [1, 2]
irb(main):004:0>

This comment has been minimized.

Show comment Hide comment
@rafaelfranca

rafaelfranca Jan 6, 2012

Owner

Yeah, but as single_or_multiple_records can be an instance of ActiveRecord::Base, to_ary will return nil and Array() will call to_a, Array.wrap not

@rafaelfranca

rafaelfranca Jan 6, 2012

Owner

Yeah, but as single_or_multiple_records can be an instance of ActiveRecord::Base, to_ary will return nil and Array() will call to_a, Array.wrap not

This comment has been minimized.

Show comment Hide comment
@tenderlove

tenderlove Jan 6, 2012

Owner

I don't understand:

irb(main):005:0> class Foo; def to_ary; nil; end end
=> nil
irb(main):006:0> Array(Foo.new)
=> [#<Foo:0x007fb6fb8a18a8>]
irb(main):007:0>
@tenderlove

tenderlove Jan 6, 2012

Owner

I don't understand:

irb(main):005:0> class Foo; def to_ary; nil; end end
=> nil
irb(main):006:0> Array(Foo.new)
=> [#<Foo:0x007fb6fb8a18a8>]
irb(main):007:0>

This comment has been minimized.

Show comment Hide comment
@rafaelfranca

rafaelfranca Jan 6, 2012

Owner
>> require 'active_support/core_ext/array/wrap'
=> true
>> class Foo; def to_ary; nil; end; def to_a; ["a"]; end; end
=> nil
>> Array(Foo.new)
=> ["a"]
>> Array.wrap(Foo.new)
=> nil
@rafaelfranca

rafaelfranca Jan 6, 2012

Owner
>> require 'active_support/core_ext/array/wrap'
=> true
>> class Foo; def to_ary; nil; end; def to_a; ["a"]; end; end
=> nil
>> Array(Foo.new)
=> ["a"]
>> Array.wrap(Foo.new)
=> nil

This comment has been minimized.

Show comment Hide comment
@tenderlove

tenderlove Jan 6, 2012

Owner

I see. That does not seem like a good feature of Array.wrap.

@tenderlove

tenderlove Jan 6, 2012

Owner

I see. That does not seem like a good feature of Array.wrap.

This comment has been minimized.

Show comment Hide comment
@rafaelfranca

rafaelfranca Jan 6, 2012

Owner

Agreed! And if in this case Array.wrap return nil it will raise a exception

@rafaelfranca

rafaelfranca Jan 6, 2012

Owner

Agreed! And if in this case Array.wrap return nil it will raise a exception

This comment has been minimized.

Show comment Hide comment
@rafaelfranca

rafaelfranca Jan 6, 2012

Owner

@tenderlove I just debugged if this case will raise exceptions with classes that inherit from ActiveRecord:Base and it doesn't.

As to_aryis a private method in ActiveRecord::Core when we call Array.wrap with a ActiveRecord::Base instance it will not respond to to_ary and Array.wrap will execute [this case](https://github.com/rails/rails/blob/master/activesupport/lib/active_support/core_ext/array/wrap.rb#L45

But in this case "Kernel#Array" may have a different behavior of "Array.wrap". If someone override to_a in an ActiveRecord object this method will not works as expected and the person may become confused.

In more places where Array.wrap can be called with an ActiveRecord object I avoided to change to Kernel#Array for this reason.

@rafaelfranca

rafaelfranca Jan 6, 2012

Owner

@tenderlove I just debugged if this case will raise exceptions with classes that inherit from ActiveRecord:Base and it doesn't.

As to_aryis a private method in ActiveRecord::Core when we call Array.wrap with a ActiveRecord::Base instance it will not respond to to_ary and Array.wrap will execute [this case](https://github.com/rails/rails/blob/master/activesupport/lib/active_support/core_ext/array/wrap.rb#L45

But in this case "Kernel#Array" may have a different behavior of "Array.wrap". If someone override to_a in an ActiveRecord object this method will not works as expected and the person may become confused.

In more places where Array.wrap can be called with an ActiveRecord object I avoided to change to Kernel#Array for this reason.

@coop

This comment has been minimized.

Show comment Hide comment
@coop

coop Jan 7, 2012

Contributor

@josevalim and @sikachu I've removed the call to capture.

Contributor

coop commented Jan 7, 2012

@josevalim and @sikachu I've removed the call to capture.

@josevalim

This comment has been minimized.

Show comment Hide comment
@josevalim

josevalim Jan 7, 2012

Contributor

/cc @tenderlove Should this be merged or not?

Contributor

josevalim commented Jan 7, 2012

/cc @tenderlove Should this be merged or not?

@tenderlove

This comment has been minimized.

Show comment Hide comment
@tenderlove

tenderlove Jan 9, 2012

Owner

I think it's OK to merge if Array.wrap is changed to Array().

Owner

tenderlove commented Jan 9, 2012

I think it's OK to merge if Array.wrap is changed to Array().

@coop

This comment has been minimized.

Show comment Hide comment
@coop

coop Jan 9, 2012

Contributor

Changing Array.wrap to Array() I am getting all record_tag_helper_test.rb tests failing with ActionView::Template::Error: undefined methodmodel_name' for NilClass:Class`. I'm going to have to wait until this evening to sit down and see what is going on.

Contributor

coop commented Jan 9, 2012

Changing Array.wrap to Array() I am getting all record_tag_helper_test.rb tests failing with ActionView::Template::Error: undefined methodmodel_name' for NilClass:Class`. I'm going to have to wait until this evening to sit down and see what is going on.

@carlosantoniodasilva

This comment has been minimized.

Show comment Hide comment
@carlosantoniodasilva

carlosantoniodasilva Jan 19, 2012

Owner

I think this pull can be closed now =).

I think this pull can be closed now =).

@rafaelfranca

This comment has been minimized.

Show comment Hide comment
@rafaelfranca

rafaelfranca Jan 19, 2012

Owner

👍

Owner

rafaelfranca commented Jan 19, 2012

👍

@josevalim josevalim closed this Jan 19, 2012

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment