Change matcher protocol #373

Merged
merged 16 commits into from Dec 3, 2013

Conversation

Projects
None yet
2 participants
@myronmarston
Member

myronmarston commented Nov 29, 2013

This is a first pass for #270. I'm pretty happy with it so far but there's still RSpec::Matcher.last_should to update. if this get s LGTM from another rspec core member I'll go ahead and merge w/o that; otherwise I'll add that to this PR.

@myronmarston

This comment has been minimized.

Show comment
Hide comment
@myronmarston

myronmarston Nov 29, 2013

Member

BTW, one thing you'll notice I'm doing differently here: I'm adding deprecation warnings to 3.0 rather than breaking things in 3.0 and adding deprecations to 2.99. While it would be nice to make the breaking change in 3.0 and not keep some old cruft around, these API changes are very likely to affect some extension gems (such as shoulda). I don't want users blocked from upgrading to 3.0 because of extension gems they use relying on the old APIs -- thus, I thought it best to retain support for the old protocol in 3.x. We'll plan to drop it in 4.0.

One thing that may surprise users is that they can get warning-free on 2.99, upgrade to 3.0, and then still get warnings. We'll need to make it clear that getting warning free on 2.99 means that their test suite should run on 3.0 w/o any failures, but it does not imply it will run w/o any deprecations.

Member

myronmarston commented Nov 29, 2013

BTW, one thing you'll notice I'm doing differently here: I'm adding deprecation warnings to 3.0 rather than breaking things in 3.0 and adding deprecations to 2.99. While it would be nice to make the breaking change in 3.0 and not keep some old cruft around, these API changes are very likely to affect some extension gems (such as shoulda). I don't want users blocked from upgrading to 3.0 because of extension gems they use relying on the old APIs -- thus, I thought it best to retain support for the old protocol in 3.x. We'll plan to drop it in 4.0.

One thing that may surprise users is that they can get warning-free on 2.99, upgrade to 3.0, and then still get warnings. We'll need to make it clear that getting warning free on 2.99 means that their test suite should run on 3.0 w/o any failures, but it does not imply it will run w/o any deprecations.

@myronmarston

This comment has been minimized.

Show comment
Hide comment
@myronmarston

myronmarston Nov 29, 2013

Member

I've decided to do the RSpec::Matchers.last_should change in a separate PR. It doesn't need the same "deprecate in 3.0 because extension gems might rely on it" treatment, so it'll be simpler to do that as a separate change

Member

myronmarston commented Nov 29, 2013

I've decided to do the RSpec::Matchers.last_should change in a separate PR. It doesn't need the same "deprecate in 3.0 because extension gems might rely on it" treatment, so it'll be simpler to do that as a separate change

@JonRowe

This comment has been minimized.

Show comment
Hide comment
@JonRowe

JonRowe Dec 2, 2013

Member

LGTM but I did find the block syntax you used for the matcher adapters a bit confusing at first...

Member

JonRowe commented Dec 2, 2013

LGTM but I did find the block syntax you used for the matcher adapters a bit confusing at first...

@myronmarston

This comment has been minimized.

Show comment
Hide comment
@myronmarston

myronmarston Dec 2, 2013

Member

LGTM but I did find the block syntax you used for the matcher adapters a bit confusing at first...

To what are you referring? I'm not seeing any blocks involved in the legacy matcher adapters...

Member

myronmarston commented Dec 2, 2013

LGTM but I did find the block syntax you used for the matcher adapters a bit confusing at first...

To what are you referring? I'm not seeing any blocks involved in the legacy matcher adapters...

@JonRowe

This comment has been minimized.

Show comment
Hide comment
@JonRowe

JonRowe Dec 2, 2013

Member

I'm referring to L30 and the corresponding L46-51 L59-68 etc

Member

JonRowe commented Dec 2, 2013

I'm referring to L30 and the corresponding L46-51 L59-68 etc

@myronmarston

This comment has been minimized.

Show comment
Hide comment
@myronmarston

myronmarston Dec 2, 2013

Member

I'm referring to L30 and the corresponding L46-51 L59-68 etc

Ah, that. That's the positive/negative handler, not the matcher adapters (hence my confusion).

Anyhow, take a look at ef10165 (and the contents of handler.rb before that file) -- that's simply to reduce duplication (there was a lot!). I'm certainly open to better ways to solve that problem, though: got any ideas?

Member

myronmarston commented Dec 2, 2013

I'm referring to L30 and the corresponding L46-51 L59-68 etc

Ah, that. That's the positive/negative handler, not the matcher adapters (hence my confusion).

Anyhow, take a look at ef10165 (and the contents of handler.rb before that file) -- that's simply to reduce duplication (there was a lot!). I'm certainly open to better ways to solve that problem, though: got any ideas?

@JonRowe

This comment has been minimized.

Show comment
Hide comment
@JonRowe

JonRowe Dec 3, 2013

Member

Sort of but I think merge this as is, and then I might have a go at refactoring it.

Member

JonRowe commented Dec 3, 2013

Sort of but I think merge this as is, and then I might have a go at refactoring it.

myronmarston added a commit that referenced this pull request Dec 3, 2013

@myronmarston myronmarston merged commit b9e21b9 into master Dec 3, 2013

1 check passed

default The Travis CI build passed
Details

@myronmarston myronmarston deleted the change-matcher-protocol branch Dec 3, 2013

@myronmarston

This comment has been minimized.

Show comment
Hide comment
@myronmarston

myronmarston Dec 3, 2013

Member

Done.

Member

myronmarston commented Dec 3, 2013

Done.

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