:try isn't working through delegation #5790

Closed
austinthecoder opened this Issue Apr 9, 2012 · 10 comments

Projects

None yet

8 participants

@austinthecoder

As you can see here, using try calls the method on the delegated object and not the delegator:

require 'delegate'
require 'active_support/core_ext/object/try'

class Fighter < SimpleDelegator
  def fighting?
    true
  end
end

class Person
  def fighting?
    false
  end
end

fighter = Fighter.new Person.new

fighter.fighting?       # true
fighter.try :fighting?  # false

I'm on ruby 1.9.3p125

@fxn
Member
fxn commented Apr 9, 2012

Not sure this is a bug, delegators are not subclasses of Object (they subclass BasicObject) and they do not respond to try as a consequence. But when you inherit from BasicObject you want precisely that, to have the bare minimum interface.

Opinions?

@jeremy
Member
jeremy commented Apr 9, 2012

Agreed - expected with BasicObject. However, why isn't SimpleDelegator delegating to Person#try, then?

@austinthecoder

Maybe a clearer definition of try would be best. Either it:

  1. always calls the method against the caller
  2. calls the method against the caller if the caller is a descendant of Object

I would bet most people's assumption is #1. Regardless, I think #1 is more useful.

@fxn
Member
fxn commented Apr 9, 2012

@jeremy Is delegating to Person right? The example calls try on the delegator. Since the delegator does not respond to try it forwards to the person instance, and that one responds to try with a :fighting? argument. It calls the method and returns false.

@austinthecoder

IMO :fighting? should be called on the caller, Figher. That's what delegation is for. Fighter should decide what to do at that point. Regardless, I think this is a good discussion and I'll be happy to know what contract I should ultimately follow.

@drogus
Member
drogus commented Apr 9, 2012

Just in case, you can look at SimpleDelegator's implementation, it uses method_missing, that's why try does not work as you would expect. I would just change the documentation to clarify, if someone wants different behavior, they can implement it in their delegators.

@stevegraham
Contributor

I agree with @fxn on behaviour. Some documentation of this couldn't hurt.

@drogus drogus added a commit that closed this issue May 20, 2012
@drogus drogus Improve docs for `try` by adding note on `BasicObject`
[ci skip] closes #5790
2575508
@drogus drogus closed this in 2575508 May 20, 2012
@arunagw arunagw pushed a commit to arunagw/rails that referenced this issue May 20, 2012
@drogus drogus Improve docs for `try` by adding note on `BasicObject`
[ci skip] closes #5790
6ef9fda
@chancancode
Member

I'm two years late to this discussion, but it doesn't look like we had a very satisfying resolution yet, so I'm reopening the discussion!

I can see the argument that this would be the expected behavior on BasicObject, but I would argue that you would expect #try to actually work on SimpleDelegator. If you look at the source, Delegator tries pretty hard for things like these – it overrides things like *_methods to add its own method, etc.

Also:

>> require 'delegate'
=> true
>> class Zomg < SimpleDelegator
>>   def to_s
>>     'zomg ' + super
>>   end
>> end
=> :to_s
>> Zomg.new("boom").to_s
=> "zomg boom"
>> Zomg.new("boom").send(:to_s)
=> "zomg boom"
>> Zomg.new("boom").public_send(:to_s)
=> "zomg boom"

As you can see, things like send and public_send also work seamlessly on a wrapped object.

So, I propose that in Rails 5 we patch Delegator to work with #try. Thoughts @jeremy @fxn @drogus @rafaelfranca @matthewd @sgrif?

@chancancode chancancode reopened this Mar 25, 2015
@matthewd
Member

👍

@sgrif
Member
sgrif commented Mar 25, 2015

Definitely less bad than the alternatives.

@chancancode chancancode added a commit that closed this issue May 19, 2015
@chancancode Nate Smith + chancancode Patch `Delegator` to work with `#try`
`Delegator` inherits from `BasicObject`, which means that it will not
have `Object#try` defined. It will then delegate the call to the
underlying object, which will not (necessarily) respond to the method
defined in the enclosing `Delegator`.

This patches `Delegator` with the `#try` method to work around the
surprising behaviour.

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