Skip to content

Commit

Permalink
don't raise NoMethodError the tried method doesn't exists
Browse files Browse the repository at this point in the history
  • Loading branch information
dmathieu committed May 21, 2011
1 parent f674aed commit 29a5aea
Show file tree
Hide file tree
Showing 2 changed files with 3 additions and 1 deletion.
2 changes: 2 additions & 0 deletions activesupport/lib/active_support/core_ext/object/try.rb
Expand Up @@ -28,6 +28,8 @@ class Object
def try(*a, &b) def try(*a, &b)
if a.empty? && block_given? if a.empty? && block_given?
yield self yield self
elsif !a.empty? && !respond_to?(a.first)
nil
else else
__send__(*a, &b) __send__(*a, &b)
end end
Expand Down
2 changes: 1 addition & 1 deletion activesupport/test/core_ext/object_and_class_ext_test.rb
Expand Up @@ -99,7 +99,7 @@ def setup
def test_nonexisting_method def test_nonexisting_method
method = :undefined_method method = :undefined_method
assert !@string.respond_to?(method) assert !@string.respond_to?(method)
assert_raise(NoMethodError) { @string.try(method) } assert_nil @string.try(method)
end end


def test_valid_method def test_valid_method
Expand Down

7 comments on commit 29a5aea

@laserlemon
Copy link
Contributor

Choose a reason for hiding this comment

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

Could somebody please explain this change?

As I understand it, the purpose of Object#try is to call a method on an object of an expected class, but that could also be nil. In what situation would you want to silently avoid a NoMethodError? If I'm trying a method that doesn't exist in my expected class, I should know about it.

@dmathieu
Copy link
Contributor Author

Choose a reason for hiding this comment

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

On STI objects where you might have methods defined for some records, but not for others for example.

@josevalim
Copy link
Contributor

@josevalim josevalim commented on 29a5aea Oct 5, 2011 via email

Choose a reason for hiding this comment

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

@jonleighton
Copy link
Member

Choose a reason for hiding this comment

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

Agreed, I think we should revert this.

@alindeman
Copy link
Contributor

Choose a reason for hiding this comment

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

-1 to this change as well. It's not acceptable to radically change the behavior at this point.

@josevalim
Copy link
Contributor

@josevalim josevalim commented on 29a5aea Oct 6, 2011 via email

Choose a reason for hiding this comment

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

@arunagw
Copy link
Member

@arunagw arunagw commented on 29a5aea Oct 6, 2011

Choose a reason for hiding this comment

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

This commit 587dd7d also need to revert the fix the failing test. Or might need a fix!

587dd7d commit is not having test.

Please sign in to comment.