Add ArgumentError to Object#in? if supplied param doesn't respond to :include? #268

Closed
wants to merge 2 commits into
from

5 participants

@jaredonline

The comment above the method specifically states the supplied param must respond to include?, but allows a method that doesn't fall through to a NoMethodError. Instead it should raise a more descriptive error.

@dhh
Ruby on Rails member

Can you benchmark this version against the vanilla one? Curious how much slower it is to have a respond and a rescue up front than not. On Ruby 1.9, the version without is only 23% slower than straight include?

@sikachu
Ruby on Rails member

I don't know if we really need to explicitly check for the error or not. I do understand that it will surely get NoMethodError for #include?, but don't know if having an error at #in? level would be neccessary.

Anyway, my suggestion for this would be cache the NoMethodError returned from #include? and change the exception message. I think in that case we won't suffer from upfront rescue.

@dhh
Ruby on Rails member

Actually, we can just catch NoMethodError and reraise an ArgumentError instead. That's the way to go. Then there's no preemptive check and thus no cost. Please make it so (and include tests as well as documentation, so people know which exception to expect.)

@sikachu
Ruby on Rails member
@jfirebaugh

I don't think we need to change this. NoMethodError is descriptive enough.

@josevalim just recently removed several instances of the type of rescue/raise combo you are suggesting: df5691a

@josevalim
Ruby on Rails member

-1. This is usually referred as chicken typing. There is no need check if it responds to something, runtime will tell us.

@dhh
Ruby on Rails member

Yes, -1 on the first implementation, but having an ArgumentError makes more sense than a NoMethodError because what's failing is that the first argument doesn't live up to what you expect. Ultimately it doesn't matter much, but it would be nicer.

@jaredonline

Okay, I added the test and changed it to catch a NoMethodError.

@jfirebaugh But if you call 1.in?(1), a NoMethodError isn't the problem. The problem is that you actually passed in a bad argument, and so the proper ArgumentError should be raised. If you get a NoMethodError, you need to know the internal implementation of #in? to understand what happened.

@dhh
Ruby on Rails member

Applied. Thanks!

@dhh dhh closed this Apr 14, 2011
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment