Add the ability to add a "chain" to an existing matcher

[benoittgt]

Subject of the issue

This is a feature request. I think it could be a good idea to let people "extend" RSpec matchers with chaining capabilities.

For exammple:

RSpec::Matchers.define_chain BuiltIn::HaveAttributes do
   chain :with_value do |actual_hash|
    expected.value == actual_hash.value
  end
end

it { expect(hash).to have_key(:id).with_value(42) }

[JonRowe]

We don't really encourage people to monkey patch or inherit from our matchers, they are not even encouraged to know the class names (by which I mean they are tagged as @api private). Depending on this was implemented it could open us up to issues where by people can accidentally make breaking changes to our api which affects how other libraries use our matchers.

So if we were going to do this, I'd really want to lookup other matchers by name, and not by the class; and I'd want the DSL to take the class and create a new one from it, apply the block and override the old implementation.

So it would look more like:

RSpec::Matchers.extend_existing :have_attributes do
   chain :with_value do |actual_hash|
    expected.value == actual_hash.value
  end
end

it { expect(hash).to have_attribute(:id).with_value(42) }

But even this still risks breaking implementations that also rely on names...

[benoittgt]

Thanks Jon for your answer. After everything you say, it seems to be a bad idea :)

Feel free to close this feature request if you think we should not accept a proposal for this.😉

[JonRowe]

If you want to investigate this further I don't mind, if you want to close I don't mind, I just wanted to point out the potential pitfalls :)

[benoittgt]

Ok. I will let the feature request as open. If no one request it, I will close it before RSpec 4. :)

[pirj]

Please correct me if I'm mistaken, but I'm under the impression this can be done with something like:

module HaveAttributesWithValue
  def with_value(value)
    @expected_value = value
  end

  def matches?(actual)
    super && @expected_value == actual_hash.value
  end
end
  
BuiltIn::HaveAttributes.prepend(HaveAttributesWithValue)

# or even better

module RSpec
  module Matchers
    class HaveAttributesEx < BuiltIn::HaveAttributes
      prepend HaveAttributesWithValue
    end

    def have_key(expected)
      HaveAttributesEx.new(expected)
    end
  end
end


it { expect(hash).to have_key(:id).with_value(42) }

[JonRowe]

The first is no better/worse than @benoittgt's ideas, the second overrides our implementation and would have the same problems with other uses, but at least it would be deliberate by the user.

[pirj]

the second overrides our implementation

Does it?

[JonRowe]

Does it?

It does if we define have_key

[pirj]

It can probably overlap with our have_* predicate matchers. However, this is just an example of adding a chainable specifier to an arbitrary matcher. Not necessary to a built-in RSpec matcher.
This technique allows both to extend an existing matcher, or to extend in a subclass without affecting an existing matcher.

Also, this doesn't have to be defined in RSpec::Matchers namespace at all. Can be in a user namespace, and included in example groups that need this matcher. I've used RSpec::Matchers for simplicity since it's included by default.

[JonRowe]

This technique allows both to extend an existing matcher, or to extend in a subclass without affecting an existing matcher.

This is my point, it doesn't prevent affecting an existing matcher when it overrides how that matcher is used. Remember people use the matcher names to access matchers, not the classes...

[pirj]

I'm sure I quite understand why you are so opposed to this technique?

Doesn't

RSpec::Matchers.define_chain BuiltIn::HaveAttributes do
   chain :with_value do |actual_hash|
    expected.value == actual_hash.value
  end
end

affect an existing matcher in the same way?

The reason I'm proposing it is that there's (probably) nothing needs to be done, and there's nothing to maintain except for adding documentation for how to do this. As opposed to define_chain will be a new feature.

[JonRowe]

I'm sure I quite understand why you are so opposed to this technique? Doesn't affect an existing matcher in the same way?

I'm not opposed to it, I'm just saying it has similar drawbacks to the other suggested implementations, so yes does affect existing matchers in the same way.

The reason I'm proposing it is that there's (probably) nothing needs to be done, and there's nothing to maintain except for adding documentation for how to do this. As opposed to define_chain will be a new feature.

Remember that classes are not a public API for our matchers, your proposed solution would still require us to make changes to our support, I'm not convinced we need to allow this personally I'm just open to @benoittgt researching it more :)