Should we support expect { }.should blah when both syntaxes are enabled?

[myronmarston]

Before the new syntax changes in RSpec 2.11, the following undocumented syntax worked but was not intended:

expect { something }.should raise_error(ArgumentError)

It worked because expect simply returned the block passed to it, but first extended the block with a module that added to and to_not as aliases for should and should_not, respectively. So, the object returned by expect was the block and should/should_not worked fine on it.

In RSpec 2.11, expect now returns an object that wraps the target of an expectation. to and to_not trigger the matcher checking against the target object, but should and should_not are not explicitly defined--so expect { }.should attempts to match against the ExpectationTarget object, not against the block that it wraps.

I don't think expect { }.should was ever intended to be supported (it was a side effect of the implementation) but it appears to have seen a fair bit of use--for example, it was used in @mhartl's rails tutorial, so people copied it (see the discussion here).

I don't have a strong feeling either way but wanted to open up this ticket for discussion. If we did add back support, I'd want to make it print a deprecation warning and we'd remove it in 3.0.

Thoughts?

/cc @alindeman @dchelimsky @mhartl

[dchelimsky]

I'm OK with adding it in the interest of good will toward readers of the tutorial, but I'd prefer to do so with a deprecation warning (less problematic than an exception) and removal in 3.0.

[dchelimsky]

@myronmarston I missed that you had already commented on deprecating it. I guess we're on the same page.

[mhartl]

My use of the syntax in the tutorial was an error, since fixed. I agree with deprecating it.

[timcheadle]

Agreed with deprecating it. "I expect to" seems pretty reasonable, whereas "I expect should" seems a little off somehow, even if grammatically correct.

I only noticed this in the first place after I wrote expect {}.to out of habit and noticed that my code differed from the examples in the Rails Tutorial (that used expect {}.should), and that those examples failed with RSpec 2.11.

[alindeman]

👍

[mhartl]

Excellent. When is this slated for release?

[myronmarston]

Excellent. When is this slated for release?

It's already been released :). I cut a 2.11.3 release last night. Here's the diff:

v2.11.2...v2.11.3

[mhartl]

Awesome. Thanks. :-)

[avit]

@myronmarston Here's a reasonable use case for keeping "expect ... should":

calling = expect { SomeKindOf::ReallyLong.method_call(with: funny..parameters) }
calling.should raise_error ArgumentError

Maybe "expect" isn't the semantically correct method name when assigning the ExpectationTarget to a variable but it definitely reads better on the second line. I suppose it could also be broken up as:

calling = proc { SomeKindOf::ReallyLong.method_call(with: funny..parameters) }
expect(&calling).to raise_error ArgumentError

(I try to avoid chaining methods on the end of really long or multi-line blocks for readability if I can avoid it.)

[myronmarston]

FWIW, my preferred code formatting for block expectations is:

expect {
  do_something
}.to raise_error(ArgumentError)

[dchelimsky]

... when assigning the ExpectationTarget to a variable

Why would you want to do that?

[avit]

@dchelimsky mainly to break things up for readability, perhaps even moving the block into a helper method. Multi-line blocks with chained methods tacked on can look odd sometimes.

doing_stuff_with(invalid_id).should raise RecordNotFound

def doing_stuff_with(*params)
  expect { described_class.do_stuff(*params) }
end

Why prevent it?

[myronmarston]

Why prevent it?

We haven't prevented it. This is ruby. You're welcome to alias to to should on RSpec::Expectations::ExpectationTarget.

What's supported out of the box with RSpec isn't meant to limit what you choose to do with it.

[avit]

Well, the should is already in there and works (as of 2.13.x), it just spits deprecation warnings... I was just making the argument for keeping it in and getting rid of all the deprecation boilerplate that's there now. Either way, I'm fine with the workaround:

class RSpec::Expectations::ExpectationTarget
  disable_deprecated_should
  alias :should :to
end

[myronmarston]

Well, the should is already in there and works (as of 2.13.x), it just spits deprecation warnings... I was just making the argument for keeping it in and getting rid of all the deprecation boilerplate that's there now.

If a majority of RSpec users wanted this, I'd definitely be open to it. As far as I know, you're the only one who has ever asked for it. On top of that, I think it would be confusing to leave an un-deprecated .should in place when folks have explicitly disabled the :should syntax.