Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Give priority to pluralised collection method in have matcher #163

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
5 participants

adzap commented Jul 27, 2012

If the target defines both the singular and plural form of a collection name the singular form will be used for the have(1) case.

target.respond_to?(:versions) #=> true
target.respond_to?(:version) #=> true, but does not return a collection

target.should have(1).version # fails because it uses version method 

Giving priority to the pluralised collection name solves this issue.

This pull request fails (merged c08dad3 into d427bac).

adzap commented Jul 27, 2012

Failure is nothin to do with my patch.

Contributor

justinko commented Jul 27, 2012

There is a discussion around deprecating have:

#93

adzap commented Jul 27, 2012

I sympathise with that. It is a bit 'magical' and messy.

On 28/07/2012, at 2:20 AM, Justin Koreply@reply.github.com wrote:

There is a discussion around deprecating have:

#93


Reply to this email directly or view it on GitHub:
#163 (comment)

Contributor

alindeman commented Dec 6, 2012

Thoughts on what to do with this one @myronmarston?

I seems reasonable, but I worry about 1) breaking folks who are accidentally depending on this behavior 2) giving attention to have which we might deprecate/remove later.

@myronmarston myronmarston commented on the diff Dec 6, 2012

spec/rspec/matchers/have_spec.rb
@@ -122,6 +122,18 @@ def self.pluralize(string)
owner.should have(1).item
end
+ context "when the target defines methods for the singular and plural form of the collection name" do
+
+ it 'uses the plural form' do
+ owner = create_collection_owner_with(1)
+ def owner.item
+ :not_a_collection
+ end
+
+ owner.should have(1).item
+ end
+ end
+
@myronmarston

myronmarston Dec 6, 2012

Owner

There's an aspect of this test which is a bit confusing: owner.item returns one item, so owner.should have(1).item looks like it may be using that method (but simply wrapping it with Array() or whatever). I assume owner.items has 1 thing in it, but the context isn't shown in the diff.

Maybe you could make owner.item raise an error to demonstrate that it's not being called?

Owner

myronmarston commented Dec 6, 2012

I worry about 1) breaking folks who are accidentally depending on this behavior

Hmmm...my first thought is that that's unlikely, and this seems like a bug now. My second thought is that maybe it's not so unlikely. I don't know. It suggests that we really should deprecate have: the method missing stuff it's doing leads to confusion like this (e.g. will it delegate to item or items?).

My inclination is to close this, and start thinking about the have deprecation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment