implement AS::Duration#send and #try to avoid unexpected behavior

[misfo]

since Duration inherits from ::BasicObject when using Ruby 1.9, send and try aren't implemented so they're delegated to the Integer value by method_missing. This causes unexpected behavior:

d = 1.day
d.inspect                #=> "1 day"
d.try(:inspect) || "N/A" #=> "86400"
# ^^^ standard null checking goes pear-shaped

[sarahhodne]

The code you submitted for try is not compliant with what try is supposed to do though (prevent whiny nils). Also, is there a reason why Duration inherits from BasicObject and not Object?

[misfo]

The commit won't affect NilClass#try at all, so it won't change how try "prevents whiny nils".

That is a good point about Duration's superclass, though. It's not clear to me why it inherits from BasicObject in the first place. Does anyone here on the Githubs know why that is?

[sarahhodne]

Doi, I had a brain fart there about NilClass#try. You're absolutely right, this is the expected way of doing try. To make it even better, is there a way you can just mix in the try and send from Object? That way you don't have to update it in two places if it changes (which might not be that plausible at all).

[misfo]

Turns out making Duration simply inherit from Object fixes all the worlds problems. Thanks for questioning that @dvyjones

I made a new pull request here:
#334

[sikachu]

Reopen after some discussion in #334

Turned out that making it inherited from Object actually have more side effect than we want. So, @misfo if you would please rebase your patch, and make it as a module that we can reuse it later, that would be awesome.

Thank you for the hard work. I'm target this one for 3.2.x

[misfo]

Sure will. Thanks for taking a look at this. I'll comment on this pull when I've created a new one...

[jeremy]

Ping. Did you make progress on this?

[misfo]

@jeremy, I looked at this again and I still think Duration should subclass Object as it did in the other pull. Duration#tap doesn't work as expected either when ActiveSupport::BasicObject is subclassed:

irb> 1.days.is_a?(ActiveSupport::Duration)
=> true
irb> 1.days.tap {|d| puts d.is_a?(ActiveSupport::Duration) }
false
=> 86400
irb> 1.days.tap {|d| d }.is_a?(ActiveSupport::Duration)
=> false

I understand that Duration should proxy a lot of its methods to its @value, but doesn't it make more sense to explicitly delegate the methods in Object.instance_methods - ActiveSupport::BasicObject.instance_methods that we want proxied. That way everything we want to be proxied is, but basic assumptions about try, send, tap, etc aren't violated.

[jeremy]

As a proxy, it's clearer to be explicit about what we don't want to be proxied, rather than what we do. It's easier to maintain, too.

This could be a good case for a better supporting class, like ActiveSupport::ProxyObject, though.

[mhfs]

@jeremy I was looking into this and it seems ActiveSupport::BasicObject (which ActiveSupport::Duration inherits from) is used only for Duration and an active resource spec. Also BasicObjects docs says "Used for proxy classes". Would be the case of renaming it into ProxyObject and placing send and try in there?

Or do you prefer Duration < ProxyObject < BasicObject?

Would be happy to provide a patch.

/cc @josevalim

[isaacsanders]

Is this still an issue @misfo?

[misfo]

Yup, it's still an issue. I made another pull request that resolves @jeremy's concerns, though. It uses an Array of methods inherited from Object that we don't wanna proxy, but avoids reimplementing methods from Object like this pull does. Check it out: #6055

[isaacsanders]

Is this okay to close?

[misfo]

Surely