ms-ati / docile

Docile keeps your Ruby DSLs tame and well-behaved
http://ms-ati.github.com/docile/
MIT License
419 stars 34 forks source link

Fix problem to catch NoMethodError from non receiver object #12

Closed le0pard closed 10 years ago

le0pard commented 10 years ago

Problem case: https://github.com/follmann/middleman-favicon-maker/issues/22#issuecomment-34058515

coveralls commented 10 years ago

Coverage Status

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

ms-ati commented 10 years ago

@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 commented 10 years ago

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 commented 10 years ago

@ms-ati One moment, almost done :)

ms-ati commented 10 years ago

@le0pard ok, sure, no problem :+1:

ms-ati commented 10 years ago

@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 commented 10 years ago

@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 commented 10 years ago

@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 commented 10 years ago

One moment, will add such spec.

ms-ati commented 10 years ago

@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 commented 10 years ago

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

le0pard commented 10 years ago

Fixed. Just need wait for travis :)

ms-ati commented 10 years ago

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

ms-ati commented 10 years ago

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

le0pard commented 10 years ago

@ms-ati give me a moment :)

ms-ati commented 10 years ago

Is it this: https://github.com/rubinius/rubinius/issues/2794 or https://github.com/travis-ci/travis-ci/issues/1641?

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

ms-ati commented 10 years ago

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 methodpush' on nil:NilClass.>`

ms-ati commented 10 years ago

@le0pard Here's how to match an exception in RSpec using regex: https://www.relishapp.com/rspec/rspec-expectations/v/2-6/docs/built-in-matchers/raise-error-matcher#expect-specific-error-message-using-a-regular-expression

le0pard commented 10 years ago

Finally! :beers:

ms-ati commented 10 years ago

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

ms-ati commented 10 years ago

Released v1.1.3 with your fix!