Blocks provided to `with` are used as implementation. #426

Merged
merged 1 commit into from Oct 2, 2013

Projects

None yet

3 participants

@xaviershay
RSpec member

It is confusing to have different behaviour depending on whether the
block has arguments or not, and there are better ways to do this.

This behaviour matches that of other message expectations.

Fixes #377.

@xaviershay
RSpec member

This is a breaking change. Deprecation added to 2-99 branch in #425.

@coveralls

Coverage Status

Coverage decreased (-0.08%) when pulling 2e7a97b on issue-377 into 8e1a653 on master.

@myronmarston myronmarston commented on an outdated diff Oct 2, 2013
lib/rspec/mocks/message_expectation.rb
@@ -353,8 +353,13 @@ def raise_out_of_order_error
# cart.add(Book.new(:isbn => 1934356379))
# # => passes
def with(*args, &block)
- self.inner_implementation_action = block if block_given? unless args.empty?
- @argument_list_matcher = ArgumentListMatcher.new(*args, &block)
+ if args.empty?
+ raise ArgumentError,
+ "`with` must have at least one argument. Use `no_args` matcher to set this expectation."
@myronmarston
myronmarston Oct 2, 2013

I think "this expectation" is a little odd (it's not clear what expectation the user is trying to set to me -- it may just be that they hit their editors key binding that ran the specs before finishing typing the expression). Maybe instead say '...to set the expectation of receiving no arguments'?

@myronmarston myronmarston and 1 other commented on an outdated diff Oct 2, 2013
spec/rspec/mocks/failing_argument_matchers_spec.rb
expect do
@double.should_receive(:msg).with {|arg| expect(arg).to eq :received }
- @double.msg :no_msg_for_you
- end.to raise_error(RSpec::Expectations::ExpectationNotMetError, /expected: :received.*\s*.*got: :no_msg_for_you/)
+ end.to raise_error(ArgumentError,
+ "`with` must have at least one argument. Use `no_args` matcher to set this expectation.")
@myronmarston
myronmarston Oct 2, 2013

FWIW, I don't usually spell out the entire exception message...I'd probably just do to raise_error(ArgumentError, /must have at least one arg/) -- that way we can tweak phrasing w/o necessarily having to tweak it here as well.

@xaviershay
xaviershay Oct 2, 2013

cool, I like doing that too but was copying another match I saw elsewhere. Will change.

@myronmarston
RSpec member

LGTM. Left a couple minor suggestions but this is good to merge otherwise.

@xaviershay xaviershay Blocks provided to `with` are used as implementation.
It is confusing to have different behaviour depending on whether the
block has arguments or not, and there are better ways to do this.

This behaviour matches that of other message expectations.
7ef80f5
@coveralls

Coverage Status

Coverage decreased (-0.08%) when pulling 7ef80f5 on issue-377 into 8e1a653 on master.

@xaviershay xaviershay merged commit 253eeed into master Oct 2, 2013

1 check passed

Details default The Travis CI build passed
@xaviershay xaviershay deleted the issue-377 branch Oct 2, 2013
@myronmarston myronmarston added a commit that referenced this pull request Nov 20, 2013
@myronmarston myronmarston Remove support for ArgumentListMatcher block.
This feature was removed in #426, but this bit of
the implementation of that feature remained.
83d34c3
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment