Open benoittgt opened 3 years ago
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...
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.😉
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 :)
Ok. I will let the feature request as open. If no one request it, I will close it before RSpec 4. :)
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) }
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.
the second overrides our implementation
Does it?
Does it?
It does if we define have_key
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.
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...
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.
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
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 :)
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: