Give users an error when they use a value matcher in a block expectation expression #530

Merged
merged 5 commits into from Apr 20, 2014

Conversation

Projects
None yet
3 participants
@myronmarston
Member

myronmarston commented Apr 17, 2014

Start of fix for #526.

  • Fix the block matchers to give a clear error when given a value rather than a block arg.
  • Add a deprecation warning to 2.99.
@myronmarston

This comment has been minimized.

Show comment
Hide comment
class ExpectationTarget
+ # @private
+ # Used as a sentinel value to be able to tell when the user

This comment has been minimized.

@JonRowe

JonRowe Apr 18, 2014

Member

I've never heard the term "sentinel value" before, I like it.

@JonRowe

JonRowe Apr 18, 2014

Member

I've never heard the term "sentinel value" before, I like it.

+ else
+ @block_expectation = false
+ @target = value
+ end

This comment has been minimized.

@JonRowe

JonRowe Apr 18, 2014

Member

So I'm thinking if we did this as a factory method, that returned either a ExpectationTarget or a BlockExpectationTarget rather than in #initialize, we could skip the conditional checks later on.

@JonRowe

JonRowe Apr 18, 2014

Member

So I'm thinking if we did this as a factory method, that returned either a ExpectationTarget or a BlockExpectationTarget rather than in #initialize, we could skip the conditional checks later on.

This comment has been minimized.

@cupakromer

cupakromer Apr 18, 2014

Member

Any reason this couldn't be part of expect syntax definition?

@cupakromer

cupakromer Apr 18, 2014

Member

Any reason this couldn't be part of expect syntax definition?

This comment has been minimized.

@myronmarston

myronmarston Apr 18, 2014

Member

So I'm thinking if we did this as a factory method, that returned either a ExpectationTarget or a BlockExpectationTarget rather than in #initialize, we could skip the conditional checks later on.

Great idea, I'll push a fix for this soon.

Any reason this couldn't be part of expect syntax definition?

Well, this version of the logic (that has to infer if it's a block expectation or not) really fits better here, as the @block_expectation variable is really internal state. I'm moving in the direction of a factory method. I don't want it in the syntax file, though; those inline method defs are intended to be short.

@myronmarston

myronmarston Apr 18, 2014

Member

So I'm thinking if we did this as a factory method, that returned either a ExpectationTarget or a BlockExpectationTarget rather than in #initialize, we could skip the conditional checks later on.

Great idea, I'll push a fix for this soon.

Any reason this couldn't be part of expect syntax definition?

Well, this version of the logic (that has to infer if it's a block expectation or not) really fits better here, as the @block_expectation variable is really internal state. I'm moving in the direction of a factory method. I don't want it in the syntax file, though; those inline method defs are intended to be short.

@@ -25,7 +46,8 @@ def initialize(target)
# @return [Boolean] true if the expectation succeeds (else raises)
# @see RSpec::Matchers
def to(matcher=nil, message=nil, &block)
- prevent_operator_matchers(:to, matcher)
+ enforce_block_matcher(matcher) if @block_expectation
+ prevent_operator_matchers(:to) unless matcher

This comment has been minimized.

@JonRowe

JonRowe Apr 18, 2014

Member

As per my previous comment, these could easily be:

def to(...)
  enforce_block_matcher(matcher)
  super
end

in a hypothetical BlockExpectationTarget

@JonRowe

JonRowe Apr 18, 2014

Member

As per my previous comment, these could easily be:

def to(...)
  enforce_block_matcher(matcher)
  super
end

in a hypothetical BlockExpectationTarget

+ # Used as a sentinel value to be able to tell when the user
+ # did not pass an argument. We can't use `nil` for that because
+ # `nil` is a valid value to pass.
+ UndefinedValue = Module.new

This comment has been minimized.

@cupakromer

cupakromer Apr 18, 2014

Member

Any particular reason for the choice of Module?

@cupakromer

cupakromer Apr 18, 2014

Member

Any particular reason for the choice of Module?

This comment has been minimized.

@myronmarston

myronmarston Apr 18, 2014

Member

For singleton objects that are assigned to a constant I prefer to use a module or a class over just Object.new because the constant assignment takes care of giving it a good to_s/inspect rather than just Object#inspect.

For Module vs Class I tend to use a module if it's not intended to be instantiated. It probably doesn't matter but a module feels more limited/lightweight to me.

@myronmarston

myronmarston Apr 18, 2014

Member

For singleton objects that are assigned to a constant I prefer to use a module or a class over just Object.new because the constant assignment takes care of giving it a good to_s/inspect rather than just Object#inspect.

For Module vs Class I tend to use a module if it's not intended to be instantiated. It probably doesn't matter but a module feels more limited/lightweight to me.

This comment has been minimized.

@cupakromer

cupakromer Apr 18, 2014

Member

Thanks for that insight. That helps me a bunch.

@cupakromer

cupakromer Apr 18, 2014

Member

Thanks for that insight. That helps me a bunch.

lib/rspec/expectations/syntax.rb
- raise ArgumentError.new("You must pass an argument or a block to #expect but not both.") unless target.size == 1
- ::RSpec::Expectations::ExpectationTarget.new(target.first)
+ def expect(value=::RSpec::Expectations::ExpectationTarget::UndefinedValue, &block)
+ ::RSpec::Expectations::ExpectationTarget.new(value, block)

This comment has been minimized.

@cupakromer

cupakromer Apr 18, 2014

Member

Per the comments above. Could this be the factory method which creates instances of the different target types?

@cupakromer

cupakromer Apr 18, 2014

Member

Per the comments above. Could this be the factory method which creates instances of the different target types?

This comment has been minimized.

@JonRowe

JonRowe Apr 18, 2014

Member

We'd want the factory method somewhere else, in general we don't like the monkey patched methods to do much.

@JonRowe

JonRowe Apr 18, 2014

Member

We'd want the factory method somewhere else, in general we don't like the monkey patched methods to do much.

@myronmarston

This comment has been minimized.

Show comment
Hide comment
@myronmarston

myronmarston Apr 18, 2014

Member

OK, I refactored based on @JonRowe's suggestion. Still have a couple more TODOs (see above).

Member

myronmarston commented Apr 18, 2014

OK, I refactored based on @JonRowe's suggestion. Still have a couple more TODOs (see above).

@myronmarston

This comment has been minimized.

Show comment
Hide comment
@myronmarston

myronmarston Apr 18, 2014

Member

Hmm, I'm actually wondering if supports_block_expectations? is better than block_matcher?. I think in the custom matcher DSL it reads better:

RSpec::Matchers.define :match_a_block do
  match { ... }
  supports_block_expectations
end

Also, block_matcher? makes it sound like a matcher that returns true from that can't also work with values, but @cupakromer had a case where it could support either.

Any preference?

Member

myronmarston commented Apr 18, 2014

Hmm, I'm actually wondering if supports_block_expectations? is better than block_matcher?. I think in the custom matcher DSL it reads better:

RSpec::Matchers.define :match_a_block do
  match { ... }
  supports_block_expectations
end

Also, block_matcher? makes it sound like a matcher that returns true from that can't also work with values, but @cupakromer had a case where it could support either.

Any preference?

@JonRowe

This comment has been minimized.

Show comment
Hide comment
@JonRowe

JonRowe Apr 18, 2014

Member

I'm ok with supports_block_expectations?

Member

JonRowe commented Apr 18, 2014

I'm ok with supports_block_expectations?

@cupakromer

This comment has been minimized.

Show comment
Hide comment
@cupakromer

cupakromer Apr 18, 2014

Member

Agree, 👍 for supports_block_expectations

Member

cupakromer commented Apr 18, 2014

Agree, 👍 for supports_block_expectations

myronmarston added some commits Apr 17, 2014

Add `supports_block_expectations?` to matcher protocol.
This is used to give users a clear error when they
wrongly use a value matcher against a block, expecting
it to match against the return value of the block:

expect { 3 }.to eq(3)
@myronmarston

This comment has been minimized.

Show comment
Hide comment
@myronmarston

myronmarston Apr 18, 2014

Member

OK, I think this is ready to go (except for the 2.99 deprecation)....care to rereview?

Member

myronmarston commented Apr 18, 2014

OK, I think this is ready to go (except for the 2.99 deprecation)....care to rereview?

@JonRowe

This comment has been minimized.

Show comment
Hide comment
@JonRowe

JonRowe Apr 20, 2014

Member

LGTM

Member

JonRowe commented Apr 20, 2014

LGTM

myronmarston added a commit that referenced this pull request Apr 20, 2014

Merge pull request #530 from rspec/block-matcher-clarification
Give users an error when they use a value matcher in a block expectation expression

@myronmarston myronmarston merged commit 1603a39 into master Apr 20, 2014

1 check passed

continuous-integration/travis-ci The Travis CI build passed
Details

@myronmarston myronmarston deleted the block-matcher-clarification branch Apr 20, 2014

HoneyryderChuck pushed a commit to HoneyryderChuck/rspec-html-matchers that referenced this pull request May 24, 2014

@bf4 bf4 referenced this pull request in metricfu/metric_fu Jun 2, 2014

Merged

Update to latest (breaking) RSpec 3 changes #232

@myronmarston myronmarston referenced this pull request Jun 19, 2014

Closed

Waiting matcher #580

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