Permalink
Browse files

Merge pull request #10347 from lellisga/delegation_bug_documentation

Delegation method bug
  • Loading branch information...
2 parents d495606 + ccbe023 commit 23b6e9d85d4a57ab0a24ae5e03965ff6c8de5bca @guilleiguaran guilleiguaran committed Apr 26, 2013
Showing with 19 additions and 0 deletions.
  1. +14 โˆ’0 activesupport/lib/active_support/core_ext/module/delegation.rb
  2. +5 โˆ’0 activesupport/test/core_ext/module_test.rb
@@ -112,6 +112,20 @@ class Module
# end
#
# Foo.new.zoo # returns 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.
+ #
+ # class Foo
+ # def initialize(bar)
+ # @bar = bar
+ # end
+ #
+ # delegate :name, to: :@bar
+ # end
+ #
+ # Foo.new("Bar").name # raises NoMethodError: undefined method `name'
+ #
def delegate(*methods)
options = methods.pop
unless options.is_a?(Hash) && to = options[:to]
@@ -171,6 +171,11 @@ def test_delegation_with_allow_nil_and_nil_value
assert_nil rails.name
end
+ def test_delegation_with_allow_nil_and_invalid_value
+ rails = Project.new("Rails", "David")
+ assert_raise(NoMethodError) { rails.name }
+ end
+
def test_delegation_with_allow_nil_and_nil_value_and_prefix
Project.class_eval do
delegate :name, :to => :person, :allow_nil => true, :prefix => true

9 comments on commit 23b6e9d

@fxn
Member
fxn commented on 23b6e9d Apr 26, 2013

I believe this mention of false is a smell and points to a bug in the code. The option is called :allow_nil, and the generated method should check for that condition, but it is doing

if client || client.respond_to?(:name)
  ...
end

I believe that should instead be

if !client.nil? || client.respond_to?(:name)
  ...
end

and the mention to false in the new RDoc be removed.

What do you think?

@pixeltrix
Member

Probably, though it's a pretty ugly condition ๐Ÿ˜„

@fxn
Member
fxn commented on 23b6e9d Apr 26, 2013

Hahaha, totally agreed I believe one is morally allowed to refactor touch typing while looking somewhere else ๐Ÿ˜„.

@jeremy
Member
jeremy commented on 23b6e9d Apr 26, 2013

@fxn Exactly, yes. I made the same comment @ #10343 (comment) ๐Ÿ˜

Also note that when we :allow_nil, we call the delegate twice now. We should assign it to a temporary variable. Crazy example: delegate :foo, to: :shift would shift once to check for truthiness then again to call shift.foo! :trollface:

@fxn
Member
fxn commented on 23b6e9d Apr 26, 2013

@jeremy good point!

@steveklabnik
Member

client.try(:respond_to, :name) ?

@fxn
Member
fxn commented on 23b6e9d Apr 26, 2013

I need to read it thrice to figure out what it does ๐Ÿ˜„.

@fxn
Member
fxn commented on 23b6e9d Apr 26, 2013

@jeremy now that I think about it, your observation reminded me of Paul Graham's "On Lisp", where he explains that multiple evaluation is a common gotcha with Lisp macros. This is the same, only with eval'ed strings.

@fxn
Member
fxn commented on 23b6e9d Apr 26, 2013

OK, I am working on a patch now for this.

Please sign in to comment.