Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Delegation method bug #10347

Merged
merged 1 commit into from Apr 26, 2013
Merged

Delegation method bug #10347

merged 1 commit into from Apr 26, 2013

Conversation

lellisga
Copy link
Contributor

Add documentation and test to delegation method that make sure we're
aware that when a delegated object is not nil or false and doesn't
respond to the method it will still raise a NoMethodError exception.

# @bar = bar
# end
#
# delegate :name, ro: :@bar
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typo - should be to: :@bar

@pixeltrix
Copy link
Contributor

@lellisga thanks for this!

@lellisga
Copy link
Contributor Author

@pixeltrix thanks to you. This is ready

@@ -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 }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fix indentation here 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

guille <3 <3 u didn't ping me "faltón" 👎

Add documentation and test to delegation method that make sure we're
aware that when a delegated object is not nil or false and doesn't
respond to the method it will still raise a NoMethodError exception.
guilleiguaran added a commit that referenced this pull request Apr 26, 2013
@guilleiguaran guilleiguaran merged commit 23b6e9d into rails:master Apr 26, 2013
@lellisga lellisga deleted the delegation_bug_documentation branch April 26, 2013 02:52
@fxn
Copy link
Member

fxn commented Apr 26, 2013

Followup: see my comment here.

fxn added a commit that referenced this pull request Apr 26, 2013
… 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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants