Bugfix: makes possible to use RSpec::Matchers#include inside a custom matcher definition

[hugobarauna]

This PR introduces a bugfix. That bug was happening because RSpec::Matchers::DSL::Matcher has a include private method that was overriding the include method included from RSpec::Matchers. (I know, too much include words in just one phrase =p)

I got that problem while trying to write the following custom matcher:

RSpec::Matchers.define :contain_products do |*products|
  match do |category|
    subcategories_products = category.subcategories.map { |sub|
      sub.products
    }
    subcategories_products.flatten!

    # calling include(*products) breaks here because it's actually calling
    # Module#include instead of RSpec::Matchers#include
    expect(subcategories_products).to include(*products)
  end
end

I think this is a bug because it is part of the spec of RSpec::Matchers::DSL::Matcher to allow us to use the built-in matchers inside a custom matcher definition.

[coveralls]

Coverage remained the same when pulling 4d78b97 on hugobarauna:master into c6fa20d on rspec:master.

[myronmarston]

@hugobarauna -- thanks for digging into this bug and providing a fix. We should definitely address this. However, there's a case that your fix won't solve:

RSpec::Matchers.define :descend_from do |module|
  match do |klass|
    expect(klass.ancestors).to include(module)
  end
end

expect(my_class).to descend_from(some_module)

I think we need to figure out a better way to detect include SomeHelperModule within the define block vs. include within match...

[hugobarauna]

@myronmarston Tks for the fast answer! I understood the problem with my solution and I would like to fix it. After your feedback I already invested one hour trying to come up with a clever solution for the problem, but I was not able to come up with anything. 😢. So, do you think you can point me a direction here?

[off-topic]
I'm writing a portuguese book about RSpec and Cucumber here in Brazil, that's one of the reasons I really would like to contribute to RSpec. I've been reading the source code the last months and I learned a lot from it.

The parts on the codebase that I'm having more difficulty to understand are the ones using a lot of context manipulation (self manipulation), like that custom matcher definition part. Lots of instance_evals here and there. It's kinda hard to understand the code execution path, but it's possible. I'm just getting used with RSpec codebase.

[hugobarauna]

@myronmarston I (think) I fixed the problem you pointed, but I didn't like that much the solution, it doesn't seen elegant. Besides that problem, I'm not sure using a instance variable for signaling if the include was called from the define or match block is a thread safe solution.

[myronmarston]

@hugobarauna -- unfortunately, this isn't an easy issue to solve :(. I'm going to have to spend some more time with it before I can come up with a suggestion for how to fix it.

You're right about the custom matcher definition being confusing. We may try to refactor it in 3.0 to not be quite as convoluted.

[hugobarauna]

Ok, if you think it's worth explaining to me a solution so to that I can code it, go for it, I would be very happy to help. But if you think this bug is too hard for a newcomer in rspec's codebase as I am, there's no problem too.

[myronmarston]

After mulling this over overnight, I'm thinking the best way to solve this is to change the way customer matchers are defined, so that rather than the main define block being instance_eval'd in an object, it defines a class and class_eval's the define block. The match block will then define the matches? instance method. This will solve the issue because within the define block it'll be a class body, where include is simply Module#include, but within the match block, it'll be run as an instance method, where include can be the include matcher. I think this should also make the match matcher work properly, making the work around in #194 unnecessary. It'll also (hopefully) make it all much less confusing, as you mentioned it is.

I think the reason it's not already defining a class in this fashion is because you would normally want to eval the define block and define the class when RSpec::Matchers.define is called, and then create an instance of that class each time the matcher is used...but the problem is, the define block yields the expected value, which isn't known until the matcher is used (and can be different each time the matcher is used), so there's no feasible way to define the class up front. Instead, we'll have to define a class each time, or consider changing the API so that the expected value is exposed in a different way (e.g. as an instance method).

Regardless, this is going to be a big enough change that I think we'll hold off until 3.0 for it.

Thoughts?

[hugobarauna]

Sorry for the delay, it's a lof of information. Unfortunately, I think I'll have the time to process that only during the mid of the next week. :(

[myronmarston]

Sorry for the delay, it's a lof of information. Unfortunately, I think I'll have the time to process that only during the mid of the next week. :(

That's fine. If you want to give it a shot later when you have the time, feel free -- that said, it's a pretty major refactoring, and something that would be pretty difficult for someone who hasn't worked with the rspec codebase much yet. So feel free to leave it to one of us -- I'll probably get to it at some point. At this point, I'm slating this for 3.0, so there's not a huge rush to get it in soon.

[myronmarston]

@hugobarauna -- I rewrote the custom matcher DSL in #332 to solve this problem (among others). I also tried to make it much less confusing. Can you take a look and let me know what you think?

Anyhow, I'm going to close this, now that we have that as a better solution. I used your spec, though :).