From 65850baf984a9349fe96bea136b81eaa36404a02 Mon Sep 17 00:00:00 2001 From: Xavier Noria Date: Fri, 26 Apr 2013 19:46:44 +0200 Subject: Module#delegate checks nilness rather that falsehood if :allow_nil is true, and avoids multiple evaluation of the target method Notes: 1) I hope nilness is a word. 2) See rationale for avoiding multiple evaluation in a comment in the patch, credit goes to @jeremy for pointing out this gotcha in the existing implementation. 3) Embeds a little joke dedicated to @pixeltrix (it could be worse! :D). References #10347. --- .../active_support/core_ext/module/delegation.rb | 72 +++++++++++++--------- 1 file changed, 42 insertions(+), 30 deletions(-) (limited to 'activesupport/lib/active_support') diff --git a/activesupport/lib/active_support/core_ext/module/delegation.rb b/activesupport/lib/active_support/core_ext/module/delegation.rb index c0828343d8..e60f9a1fe6 100644 --- a/activesupport/lib/active_support/core_ext/module/delegation.rb +++ b/activesupport/lib/active_support/core_ext/module/delegation.rb @@ -1,8 +1,10 @@ class Module - # Provides a delegate class method to easily expose contained objects' public methods - # as your own. Pass one or more methods (specified as symbols or strings) - # and the name of the target object via the :to option (also a symbol - # or string). At least one method and the :to option are required. + # Provides a +delegate+ class method to easily expose contained objects' + # public methods as your own. + # + # The macro receives one or more method names (specified as symbols or + # strings) and the name of the target object via the :to option + # (also a symbol or string). # # Delegation is particularly useful with Active Record associations: # @@ -89,39 +91,40 @@ class Module # invoice.customer_name # => 'John Doe' # invoice.customer_address # => 'Vimmersvej 13' # - # If the delegate object is +nil+ an exception is raised, and that happens - # no matter whether +nil+ responds to the delegated method. You can get a - # +nil+ instead with the +:allow_nil+ option. + # If the target is +nil+ and does not respond to the delegated method a + # +NoMethodError+ is raised, as with any other value. Sometimes, however, it + # makes sense to be robust to that situation and that is the purpose of the + # :allow_nil option: If the target is not +nil+, or it is and + # responds to the method, everything works as usual. But if it is +nil+ and + # does not respond to the delegated method, +nil+ is returned. # - # class Foo - # attr_accessor :bar - # def initialize(bar = nil) - # @bar = bar - # end - # delegate :zoo, to: :bar + # class User < ActiveRecord::Base + # has_one :profile + # delegate :age, to: :profile # end # - # Foo.new.zoo # raises NoMethodError exception (you called nil.zoo) + # User.new.age # raises NoMethodError: undefined method `age' # - # class Foo - # attr_accessor :bar - # def initialize(bar = nil) - # @bar = bar - # end - # delegate :zoo, to: :bar, allow_nil: true + # But if not having a profile yet is fine and should not be an error + # condition: + # + # class User < ActiveRecord::Base + # has_one :profile + # delegate :age, to: :profile, allow_nil: true # end # - # Foo.new.zoo # returns nil + # User.new.age # nil # - # If the delegate object is not +nil+ or +false+ and the object doesn't - # respond to the delegated method it will raise an exception. + # Note that if the target is not +nil+ then the call is attempted regardless of the + # :allow_nil option, and thus an exception is still raised if said object + # does not respond to the method: # # class Foo # def initialize(bar) # @bar = bar # end # - # delegate :name, to: :@bar + # delegate :name, to: :@bar, :allow_nil => true # end # # Foo.new("Bar").name # raises NoMethodError: undefined method `name' @@ -156,22 +159,31 @@ class Module # methods still accept two arguments. definition = (method =~ /[^\]]=$/) ? 'arg' : '*args, &block' + # The following generated methods call the target exactly once, storing + # the returned value in a dummy variable. + # + # Reason is twofold: On one hand doing less calls is in general better. + # On the other hand it could be that the target has side-effects, + # whereas conceptualy, from the user point of view, the delegator should + # be doing one call. if allow_nil - module_eval(<<-EOS, file, line - 2) + module_eval(<<-EOS, file, line - 3) def #{method_prefix}#{method}(#{definition}) # def customer_name(*args, &block) - if #{to} || #{to}.respond_to?(:#{method}) # if client || client.respond_to?(:name) - #{to}.#{method}(#{definition}) # client.name(*args, &block) + _ = #{to} # _ = client + if !_.nil? || nil.respond_to?(:#{method}) # if !_.nil? || nil.respond_to?(:name) + _.#{method}(#{definition}) # _.name(*args, &block) end # end end # end EOS else exception = %(raise "#{self}##{method_prefix}#{method} delegated to #{to}.#{method}, but #{to} is nil: \#{self.inspect}") - module_eval(<<-EOS, file, line - 1) + module_eval(<<-EOS, file, line - 2) def #{method_prefix}#{method}(#{definition}) # def customer_name(*args, &block) - #{to}.#{method}(#{definition}) # client.name(*args, &block) + _ = #{to} # _ = client + _.#{method}(#{definition}) # _.name(*args, &block) rescue NoMethodError # rescue NoMethodError - if #{to}.nil? # if client.nil? + if _.nil? # if _.nil? #{exception} # # add helpful message to the exception else # else raise # raise -- cgit v1.2.3