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

Bring try! into parity with try. #17361

Merged
merged 1 commit into from Oct 22, 2014
Merged

Bring try! into parity with try. #17361

merged 1 commit into from Oct 22, 2014

Conversation

aripollak
Copy link
Contributor

Based on commit 5e51bdd.

@chancancode
Copy link
Member

Wouldn't we need the arity test here?

class A
  def lol(obj)
    obj.try { |thing| call_me_maybe; thing.call_me_maybe }
  end

  def call_me_maybe
    puts 'method in class A'
  end
end

class B
  def call_me_maybe
    puts 'method in class B'
  end
end

A.new.lol(B.new)

I believe that your patch would change this behavior (would be nice if you could add a test like this for both try and try!)

@chancancode
Copy link
Member

Nvm, I missed that you are just delegating to try

chancancode added a commit that referenced this pull request Oct 22, 2014
@chancancode chancancode merged commit 07b80c1 into rails:master Oct 22, 2014
@chancancode
Copy link
Member

In case anyone is wondering about the same thing..

I saw that the original implementation was duplicated from try instead of delegated like what @amatsuda implemented here, so I was wondering if that's done for performance reasons.

Turns out that the difference is very small, almost smaller than what the benchmark library is capable of measuring. If you find anything wrong with my benchmarks feel free to open a new issue/PR 😄

@aripollak
Copy link
Contributor Author

It occurs to me now that it probably makes more sense to do the delegation the other way around, and just have try do the extra respond_to check. I'll look into making another PR.

@chancancode
Copy link
Member

@aripollak 👍

@aripollak aripollak deleted the try-bang-parity branch October 23, 2014 00:51
@aripollak aripollak mentioned this pull request Oct 24, 2014
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