Skip to content

Deprecation for #420 #448

Merged
merged 4 commits into from Feb 16, 2014

3 participants

@JonRowe
RSpec member
JonRowe commented Feb 9, 2014

I thought I'd already opened a PR for this, oops, anyway this is a deprecation for #420...

@JonRowe
RSpec member
JonRowe commented Feb 9, 2014

I need some help with this because there's an issue with our deprecation matcher code when multiple things are deprecated...

@JonRowe JonRowe referenced this pull request in rspec/rspec-core Feb 9, 2014
Merged

Allow custom grouping of deprecations #1307

@JonRowe
RSpec member
JonRowe commented Feb 9, 2014

(It's super noisy deprecation wise because the messages are similar but not identical)

@xaviershay xaviershay and 1 other commented on an outdated diff Feb 9, 2014
lib/rspec/matchers/matcher.rb
@@ -36,6 +36,14 @@ def initialize(name, &declarations)
# @api private
def for_expected(*expected)
+ if expected.size == 1
+ RSpec.warn_deprecation(
@xaviershay
RSpec member
xaviershay added a note Feb 9, 2014

non-blocking, but elsewhere we do:

RSpec.warn_deprecation(<<-EOS.gsub(/^\s*\|/, ''))
  |Some message
  |It's multiline!
EOS
@xaviershay
RSpec member
xaviershay added a note Feb 9, 2014

Doesn't look like there is a spec for this either?

@JonRowe
RSpec member
JonRowe added a note Feb 9, 2014

Nope, wanted to get the existing ones working before adding... (I was testing this against my scratch install manually)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@myronmarston myronmarston and 1 other commented on an outdated diff Feb 9, 2014
lib/rspec/matchers/matcher.rb
@@ -36,6 +36,14 @@ def initialize(name, &declarations)
# @api private
def for_expected(*expected)
+ if expected.size == 1
+ RSpec.warn_deprecation(
+ "Custom matchers in 3.x will set expected to be a single value "+
+ "(when provided as such) rather than an array. This may change "+
+ "the behaviour of your matcher.\n"+
+ "Called from: #{ RSpec::CallerFilter.first_non_rspec_line }\n\n"
+ )
+ end
@myronmarston
RSpec member
myronmarston added a note Feb 9, 2014

It looks like this will print a deprecation warning for each use of a custom matcher that receives a single argument. However, in the vast majority of cases it will work fine without any changes. (Plus there's nothing the user can do about this).

The problem isn't custom matchers that receive a single argument; the problem is custom matchers that access the expected attribute and expect it to be an array when it will be a single value on rspec 3. So, I think the deprecation warning needs to go in the definition of the expected method as that's the real case we care about.

We need to provide a way for users to deal with this during the upgrade process as well. (Giving them a warning, without providing a solution, does nothing). I'm not sure what's best here, but how about adding an expected_array attribute that contains the original array and will always be an array in 2.99 and 3.x? Then the deprecation warning can tell them to switch to that. We'll have to add it to 3.0 though.

Another complexity: we need to ensure the deprecation isn't issued when we access expected (e.g. from the diffing). Maybe as a temporary 2.99 measure the solution is to update the spot where we access expected to access expected_array if the method is there.

@JonRowe
RSpec member
JonRowe added a note Feb 9, 2014

@xaviershay has merged #1307 so I can compress the amount of deprecations issued with a :type key, I'm less sure about the array idea though...

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

How's this coming, @JonRowe? I think this is the only thing blocking us from releasing beta2. Let me know if I can help.

@JonRowe
RSpec member
JonRowe commented Feb 12, 2014

@myronmarston updated with a few changes

@myronmarston
RSpec member

Looking much better.

I believe we access expected from fail_with or the differ, right? In that case it should not warn but with this code it will.

@JonRowe
RSpec member
JonRowe commented Feb 14, 2014

Added suppression of this when called from the differ, unless you have a better idea? Also I couldn't figure a decent place to spec this from.

@myronmarston
RSpec member

Given that expected_as_array doesn't issue a warning, and returns the same thing on 2.99 as expected, can't you just call that from the differ rather than having the flag?

Also, I would recommend using if RSpec::Matchers::DSL::Matcher === matcher rather than if match.respond_to?(:expected_as_array). If a user happened to define an expected_as_array method on one of their matchers but was using a standalone class (rather than the custom matcher DSL) we shouldn't treat it this way, right? After all, we don't intend expected_as_array to be a generalized protocol that other matcher classes should define. The different differ method call is really only needed specifically for DSL-generated custom matchers, so we should make that explicit.

@myronmarston
RSpec member

Also I couldn't figure a decent place to spec this from.

I think a spec would belong in spec/matchers/matcher_spec.rb. I would spec it by defining a custom matcher that receives a single expected value, and calls diffable in the DSL block so that it will be diffed. Then make a failing expectation -- it should trigger the differ code path. You can use expect_no_deprecation (or whatever the helper method is) to assert that no deprecation is issued.

@JonRowe
RSpec member
JonRowe commented Feb 14, 2014

Given that expected_as_array doesn't issue a warning, and returns the same thing on 2.99 as expected, can't you just call that from the differ rather than having the flag?

No, that would reopen the original bug...

@myronmarston
RSpec member

No, that would reopen the original bug...

The original bug isn't fixed in this branch. It's fixed in 3.0. In this branch, expected and expected_as_array are identical except the deprecation issue that is issued from expected when it has only one element. Switching to the method that doesn't issue a warning (expected_as_array) doesn't affect behavior except to prevent the warning from being issued.

@JonRowe
RSpec member
JonRowe commented Feb 15, 2014

Oh of course, sorry, updated.

@myronmarston
RSpec member

LGTM

@JonRowe JonRowe merged commit 5613e31 into 2-99-maintenance Feb 16, 2014

1 check passed

Details default The Travis CI build passed
@JonRowe JonRowe deleted the deprecation_for_matcher_dsl branch Feb 16, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.