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

Fix problem to catch NoMethodError from non receiver object #12

Merged
merged 12 commits into from Feb 4, 2014
Merged

Fix problem to catch NoMethodError from non receiver object #12

merged 12 commits into from Feb 4, 2014

Conversation

le0pard
Copy link
Contributor

@le0pard le0pard commented Feb 4, 2014

from non receiver object
@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling 7396e85 on le0pard:patch-1 into 242dfda on ms-ati:master.

@ms-ati
Copy link
Owner

ms-ati commented Feb 4, 2014

@le0pard Wow, that's not a use case I foresaw, that the receiver would implement the method but return NoMethodError on purpose ;)

Thanks for the fix! Unfortunately it breaks all the 1.8.x builds. I'll investigate a bit, but feel free to also fix in the meantime for 1.8.x if you get to it first.

As soon as fixed for 1.8.x, I'll merge and do a release.

@ms-ati
Copy link
Owner

ms-ati commented Feb 4, 2014

Ah, ok, I see the issue. I'll just go ahead and merge this, and then layer a more complete fix on top of it.

@le0pard
Copy link
Contributor Author

le0pard commented Feb 4, 2014

@ms-ati One moment, almost done :)

@ms-ati
Copy link
Owner

ms-ati commented Feb 4, 2014

@le0pard ok, sure, no problem 👍

@ms-ati
Copy link
Owner

ms-ati commented Feb 4, 2014

@le0pard Oops, you have to use mime-type 1.25 on Ruby 1.8.x, do you want to do another conditional on that version?

@ms-ati
Copy link
Owner

ms-ati commented Feb 4, 2014

@le0pard do you want to try:

  if @__receiver__.respond_to?(method.to_sym)

Also, pretty please, could we have a test that fails without this fix?

@ms-ati
Copy link
Owner

ms-ati commented Feb 4, 2014

@le0pard ok, everything passes on 1.8.7 on my end with that one.

May I suggest that you add a spec (since you already changed the format of all of them!) which simply sets up the analogous situation: the fallback object calls a non-existent method, such as 42.not_a_method, and then we assert that the correct NoMethodError is thrown all the way up?

If you don't want to add it, that's fine -- I can just add this spec after merging -- let me know.

@le0pard
Copy link
Contributor Author

le0pard commented Feb 4, 2014

One moment, will add such spec.

@ms-ati
Copy link
Owner

ms-ati commented Feb 4, 2014

@le0pard

Perhaps change the gemspec section to something like:

  if defined?(RUBY_ENGINE) && 'rbx' == RUBY_ENGINE
    s.add_development_dependency 'rubysl'
    s.add_development_dependency 'rubinius-coverage'
  end

  if !(defined?(RUBY_ENGINE) && 'jruby' == RUBY_ENGINE)
    # Github flavored markdown in YARD documentation
    # http://blog.nikosd.com/2011/11/github-flavored-markdown-in-yard.html
    s.add_development_dependency 'yard'
    s.add_development_dependency 'redcarpet', '2.3.0' # because 1.8
    s.add_development_dependency 'github-markup'
  end

@ms-ati
Copy link
Owner

ms-ati commented Feb 4, 2014

Or let me know if you'd prefer that I do it, that's fine too.

@le0pard
Copy link
Contributor Author

le0pard commented Feb 4, 2014

Fixed. Just need wait for travis :)

@ms-ati
Copy link
Owner

ms-ati commented Feb 4, 2014

Excellent job, thanks so much for chasing this down and getting it fixed! I'll merge and do a release.

@ms-ati
Copy link
Owner

ms-ati commented Feb 4, 2014

@le0pard do you want to fix Rubinius now, or have you had enough for today? ;)

@le0pard
Copy link
Contributor Author

le0pard commented Feb 4, 2014

@ms-ati give me a moment :)

@ms-ati
Copy link
Owner

ms-ati commented Feb 4, 2014

Is it this: rubinius/rubinius#2794 or travis-ci/travis-ci#1641?

It looks like you just need to change travis.yml to have rbx instead of rbx-2.2.0?

@ms-ati
Copy link
Owner

ms-ati commented Feb 4, 2014

Great. Last bit: in your new spec, you need the string to be a regex with (on|for), since on Rubinius, the exception is #<NoMethodError: undefined method push' on nil:NilClass.>`

@ms-ati
Copy link
Owner

ms-ati commented Feb 4, 2014

@le0pard
Copy link
Contributor Author

le0pard commented Feb 4, 2014

Finally! 🍻

@ms-ati
Copy link
Owner

ms-ati commented Feb 4, 2014

Cheers man, I'll buy you a beer when you're next in Boston ;)

ms-ati added a commit that referenced this pull request Feb 4, 2014
Fix problem to catch NoMethodError from non receiver object
@ms-ati ms-ati merged commit 5e510f9 into ms-ati:master Feb 4, 2014
@ms-ati
Copy link
Owner

ms-ati commented Feb 4, 2014

Released v1.1.3 with your fix!

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