Deprecates the have, have_at_least, have_at_most matchers #309

hugobarauna · 2013-08-18T20:35:01Z

As planned for the RSpec 3.0 release, the have, have_at_least and have_at_most matchers aren't going to be part of the default RSpec matchers set. So, that PR deprecates those matchers.

This PR is related to #307

move the deprecations to the have matcher
print smart deprecation messages
do not print deprecations if the the rspec-collection_matchers gem is being used
refactor code following Myron's suggestions

hugobarauna · 2013-08-18T20:35:20Z

@myronmarston Please take a look at this.

myronmarston · 2013-08-19T05:41:17Z

lib/rspec/matchers.rb

@@ -460,6 +460,8 @@ def exist(*args)
    #   # Passes if "this string".length == 11
    #   expect("this string").to have(11).characters #"characters" is pure sugar
    def have(n)
+      RSpec.deprecate("`have`", :replacement => "the rspec-collection_matchers gem")
+


A few things:

By putting this here, it causes a deprecation warning to be printed if have is called, but not if BuildIn::Have is used directly. While have is the main public API, it's possible that there may be extension gems that use the matcher class directly since they may have code in a context where they don't want to mixin RSpec::Matchers to gain access to the matcher API methods like have. I'd prefer to see this warning moved into BuiltIn::Have so that it is printed regardless of if they use it directly or use this API.

The rspec-collection_matchers gem is not the only replacement. User can also rewrite an expression like expect(array).to have(10).items to expect(array.count).to eq(10). I'd like for the deprecation warning to mention that they can either install your gem or change to that kind of expression. Ideally, the deprecation warning would smartly print the exact expression they can use instead. (taking into account owned vs unowned cases)...which suggests it'll have to be in the matcher, anyway, so that it can be a bit smarter.

If users install your gem will they still get this deprecation warning? It's important that they don't, because what we'll be telling users is "upgrade to 2.99, get your spec suite warning-free, then upgrade to 3.0" -- so it's important that when they do the right thing (e.g. switch to your gem), they no longer receive a warning.

I started working on that and I wrote the following specs:

context "deprecations" do it "prints a specific message when the target is a collection" do replacement_message = "the rspec-collection_matchers gem " + "or replace your expectation with something like " + "`expect([1, 2, 3].size).to eq(3)`" expect(RSpec).to receive(:deprecate).with("`have`", :replacement => replacement_message) expect([1, 2, 3]).to have(3).items end it "prints a specific message when the target owns a collection" do class BagOfWords attr_reader :words def initialize(words) @words = words end end replacement_message = "the rspec-collection_matchers gem " + "or replace your expectation with something like " + "`expect(your_collection.words).to eq(3)`" expect(RSpec).to receive(:deprecate).with("`have`", :replacement => replacement_message) bag = BagOfWords.new(%w(foo bar baz)) expect(bag).to have(3).words end end

I was able to make those specs green but I'm having problems when the expectation is in the negative form, like:

expect(bag).to_not have(3).words

Since the matcher instance doesn't have a reference to the ExpectationHandler that called it, it can't know if the expectation is in the positive (expect(...).to) or negative (expect(...).to_not) form, so it can't know if the hint in the deprecation message should be expect(your_collection.words).to eq(3) or expect(your_collection.words).to_not eq(3) for example.

Do you have any suggestion on how to solve that problem?

Since the matcher instance doesn't have a reference to the ExpectationHandler that called it, it can't know if the expectation is in the positive (expect(...).to) or negative (expect(...).to_not) form, so it can't know if the hint in the deprecation message should be expect(your_collection.words).to eq(3) or expect(your_collection.words).to_not eq(3) for example.

This is exactly why the matcher protocol includes an optional does_not_match? method for when you need slightly different logic for the negative case (beyond simply negating matches?). For an example, see the respond_to matcher:

rspec-expectations/lib/rspec/matchers/built_in/respond_to.rb

Lines 10 to 17 in 4ccacb8

def matches?(actual)

find_failing_method_names(actual, :reject).empty?

end

alias == matches?

def does_not_match?(actual)

find_failing_method_names(actual, :select).empty?

end

JonRowe · 2013-08-19T09:35:04Z

It'd be good to get some spec's asserting on the output too :)

…:Have is being used

hugobarauna · 2013-08-27T01:43:11Z

@myronmarston @JonRowe Please take a look again. I addressed your suggestions.

myronmarston · 2013-08-27T05:09:48Z

lib/rspec/matchers/built_in/have.rb

@@ -29,13 +32,20 @@ def matches?(collection_or_owner)
            for query_method in QUERY_METHODS
              next unless collection.respond_to?(query_method)
              @actual = collection.__send__(query_method)
-              break unless @actual.nil?
+
+              if !@actual.nil?


Why not just if @actual? Seems simpler and more idiomatic. What you have here amounts to a double negative.

Done on e35f610

hugobarauna · 2013-09-14T21:06:02Z

I would like to give you an update about this PR.

Sorry for did not have finished this yet. I've been very busy these last two weeks and I'll be busy at least for one more week. I think in 2 weeks I'll be able to continue working on that.

I would like to know if there's a problem with that, regarding RSpec's 2.99 and 3 release planning.

JonRowe · 2013-09-14T23:18:28Z

I don't think thats a problem... We're unlikly to get close to releasing 2.99 or 3 in 2 weeks :) Thanks for the hard work :)

myronmarston · 2013-10-10T04:41:39Z

@hugobarauna -- where are things at for this PR? Would be great to get this wrapped up soon.

hugobarauna · 2013-10-14T15:56:05Z

@myronmarston Well asked, I've been very busy the last weeks, but I'll continue to work on that PR this week, probably tomorrow.

…tations into 2-99-maintenance Conflicts: Changelog.md

…meter

hugobarauna · 2013-10-16T01:13:27Z

Guys, I already did some of the fixes you've asked. I'm planning to continue working on that the day after tomorrow.

I think I'll be able to finish the whole PR till the end of the week.

myronmarston · 2013-10-16T03:15:36Z

lib/rspec/matchers/built_in/have.rb

+          deprecation_message << "or replace your expectation with something like "
+          deprecation_message << "`expect(#{cardinality_expression(query_method)}).#{expectation_format_method} #{matcher_expression}`"
+
+          RSpec.deprecate("`#{matcher_method}`", :replacement => deprecation_message)


I think that the matcher method on its own doesn't give much context. Instead, I think it's better if the deprecation included a matcher expression, e.g.:

expression = if @negative_expectation RSpec::Expectations::Syntax.negative_expression(cardinality_expression(query_method), old_matcher_expression) else RSpec::Expectations::Syntax.positive_expression(cardinality_expression(query_method), old_matcher_expression) end RSpec.deprecate("`#{expression}`", :replacement => deprecation_message)

The negative_expression and positive_expression methods take care of constructing a .should or expect expression based on configured settings.

Just to make sure I understood, when someone writes this: expect(owner).to have(3).items, he should get a deprecation message like that: expect(your_object).to have(3).items is deprecated. Use the rspec-collection_matchers gem or replace your expectation with something like expect(your_object.items.count).to eq(3)

right?

Done on 7bbb4c9. Take a look at it and tell me if it's ok.

hugobarauna · 2013-10-18T02:06:17Z

@myronmarston @JonRowe guys, please take a look again. I tried to address all of your suggestions.

JonRowe · 2013-10-18T04:55:01Z

Think my concerns were addressed so deferring to @myronmarston

Deprecates the have, have_at_least, have_at_most matchers

myronmarston · 2013-10-18T05:42:36Z

Thanks, @hugobarauna -- these deprecation messages will be very useful and clear for our users!

…ugobarauna/2-99-maintenance Deprecates the have, have_at_least, have_at_most matchers --- This commit was imported from rspec/rspec-expectations@591dd95.

Deprecates the have, have_at_least, have_at_most matchers

1a9cb86

myronmarston reviewed Aug 19, 2013
View reviewed changes

hugobarauna added 4 commits August 26, 2013 21:50

Move the have matcher deprecations to BuiltIn::Have

8b65fae

Explain both options for the have(n).item deprecation on the Changelog

0dafb6e

Don't print have matcher deprecations when RSpec::CollectionMatchers:…

ac499d4

…:Have is being used

Use the correct method name in the have matcher deprecation

2974f04

myronmarston reviewed Aug 27, 2013
View reviewed changes

yujinakayama mentioned this pull request Sep 23, 2013

Support conversion of collection cardinality matchers yujinakayama/transpec#5

Closed

This was referenced Sep 23, 2013

Extract its into external rspec-its gem rspec/rspec-core#1083

Closed

Add deprecation warning to 2-99-maintenance for use of collection cardinality matchers #307

Closed

hugobarauna added 2 commits October 15, 2013 20:24

Merge branch '2-99-maintenance' of git://github.com/rspec/rspec-expec…

35fa54f

…tations into 2-99-maintenance Conflicts: Changelog.md

Fix wrong merge conflict resolution

0da9758

hugobarauna added 4 commits October 15, 2013 20:35

Improve consistency in boolean variables values

e35f610

Stop creating unecessary string objects

2057dbe

Set the expectation_format_method as an ivar instead of a method para…

2b9a6ad

…meter

Break print_deprecation_message in more methods

164042a

myronmarston reviewed Oct 16, 2013
View reviewed changes

hugobarauna added 4 commits October 17, 2013 22:01

Give more context on deprecation messages

7bbb4c9

Improve the target expression in the deprecation message

d669f4a

Assert that the call_site printed in the deprecation message is right

afb03b0

Stop detecting if rspec-collection_matchers is being used

ac42a77

myronmarston added a commit that referenced this pull request Oct 18, 2013

Merge pull request #309 from hugobarauna/2-99-maintenance

591dd95

Deprecates the have, have_at_least, have_at_most matchers

myronmarston merged commit 591dd95 into rspec:2-99-maintenance Oct 18, 2013

hugobarauna deleted the 2-99-maintenance branch October 18, 2013 13:28

abotalov mentioned this pull request Sep 16, 2014

Collection-size expectation vs. collection-member count expectation rubocop/ruby-style-guide#363

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Deprecates the have, have_at_least, have_at_most matchers #309

Deprecates the have, have_at_least, have_at_most matchers #309

hugobarauna commented Aug 18, 2013

hugobarauna commented Aug 18, 2013

myronmarston Aug 19, 2013

hugobarauna Aug 22, 2013

myronmarston Aug 22, 2013

JonRowe commented Aug 19, 2013

hugobarauna commented Aug 27, 2013

myronmarston Aug 27, 2013

JonRowe Aug 27, 2013

hugobarauna Oct 15, 2013

hugobarauna commented Sep 14, 2013

JonRowe commented Sep 14, 2013

myronmarston commented Oct 10, 2013

hugobarauna commented Oct 14, 2013

hugobarauna commented Oct 16, 2013

myronmarston Oct 16, 2013

hugobarauna Oct 18, 2013

hugobarauna Oct 18, 2013

hugobarauna commented Oct 18, 2013

JonRowe commented Oct 18, 2013

myronmarston commented Oct 18, 2013

	def matches?(actual)
	find_failing_method_names(actual, :reject).empty?
	end
	alias == matches?

	def does_not_match?(actual)
	find_failing_method_names(actual, :select).empty?
	end

Deprecates the have, have_at_least, have_at_most matchers #309

Deprecates the have, have_at_least, have_at_most matchers #309

Conversation

hugobarauna commented Aug 18, 2013

hugobarauna commented Aug 18, 2013

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JonRowe commented Aug 19, 2013

hugobarauna commented Aug 27, 2013

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hugobarauna commented Sep 14, 2013

JonRowe commented Sep 14, 2013

myronmarston commented Oct 10, 2013

hugobarauna commented Oct 14, 2013

hugobarauna commented Oct 16, 2013

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hugobarauna commented Oct 18, 2013

JonRowe commented Oct 18, 2013

myronmarston commented Oct 18, 2013