-
-
Notifications
You must be signed in to change notification settings - Fork 395
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Provide a cleaner delineation between block vs value expectations #526
Comments
Me too, I think we should force usage of the block for block matchers.
That was going to be my suggestion, I think that's "better" than magically inferring which matchers support block usage or not. (Note I'm ok with matchers being allowed for both block and non block usage, but they must manage it themselves). |
I agree with what @JonRowe stated. Those were going to be my comments as well. @myronmarston were you thinking of a query method such as |
One thing that concerns me is that if we do this, it'll break any custom block matchers created by users or 3rd parties. I guess we can add a deprecation warning to 2.99, though. |
@myronmarston, @JonRowe had mentioned:
In general I tend to agree with this. Checking my work projects, I thought I had done this more, but it appears I've only done this once. It was for a rails app where I added a |
It sounds like you're talking about this: expect(response).to have_status { } ...which would be unaffected. The block vs non block we're talking about is |
@myronmarston I was referring to it "accepts a response object" do
get :index
expect(response).to have_status :not_found
end
it "allows an action proc" do
expect{ get :index }.to have_status :not_found
end As I've proven to myself, this isn't widely used. Though, I'm increasing a fan of the |
Hmm, yours is the first I've seen to support either. It seems valid to support that (even though it's non-standard) so we'd want the new method to provide a way to signal one of three values: that it requires a block, allows a block, or does not allow a block. We'll need to think more about this to come up with the right interface. |
Based on above, I'm thinking that @cupakromer's suggestion of adding two methods is best, that way theres an explicit match for |
Mostly thinking out loud here. How about using three methods: I'm not happy with the names so feel free to change them. To support backwards compatibility, and future extension, we always call def matches?(thing)
if respond_to?(:matches_object?)
raise "oops" if Proc === thing
matches_object?(thing)
elsif respond_to?(:matches_with_block?)
raise "oops" unless Proc === thing
matches_with_block?(thing)
end
end I'm not happy with that code either, but it demos the general idea. The If a matcher author wants to support both styles, they simply write a custom Caveats:
Eh.... |
I think we'd just have |
I don't want to add a conditional check to matches like you propose because of the overhead of hitting it everytime, I think it's better to rescue |
There's something about adding new I have another idea, though: a new |
That was my first instinct, but I couldn't think of a way that didn't involve a conditional check on every single matcher call, which I instantly took a disliking to. |
I think that's far better than rescuing |
After doing some more thinking, I'm leaning towards a single Originally, I was thinking that This has some nice implications:
Any objections? |
I like this plan. |
Cool, I'll take a stab at this soon. |
I too think this is a good approach. |
I've occasionally heard confusion from users about when to use
expect {...}
vsexpect(...)
. Most people I've talked to haven't been confused (the matchers that need a block obviously need a block when you stop and think about it) but I can understand the confusion, particularly for users who are new to ruby (or even programming) and don't understand the reasons why particular matchers require blocks but others don't. One example of this confusion is in #525.Currently, if you use the wrong form, you can get confusing errors or false positives/negatives. For example, passing an arg rather than a block to
yield_control
raises a confusing error:...but passing an arg to
raise_error
can result in a passing expectation:Meanwhile, when you pass a block rather than an arg (intending for the expectation to be set on the return value of the block), you can get false positives:
So, I'd like to try to come up with a way to give the user good error messages that warn them they've done the wrong thing in these situations. At a minimum, we should fix the block matchers so they fail with a consistent message when no block has been passed; then we could tell users "if you're confused, always pass an argument, and the matcher will tell you if you need a block instead". It would be nice to also give the user an error if they used a block for a value matcher, but that's much more complicated.
For starters, the block form is just syntactic sugar; the following are equivalent:
And in fact, you can use the block form in place of passing a lambda object for matchers that do not normally expect blocks:
...works just fine, although it's a bit of a tautology.
I think I'm OK with no longer allowing these to be used interchangeably (as you generally intend one or the other, and if you have a lambda or proc object you want to use with a block matcher, you can always prefix it with
&
to pass as a block), but it would be a bit of a breaking change that would need to go in 3.0.Once we do that, it would be good to provide an error for when users wrongly pass a block...but I haven't figured out a good way to do that. Ideas:
matches?
, query the proxy object to see if it was called, and raise an error if it was not -- the fact that the block was not called means the matcher didn't expect a block and the user has made a mistake. The problem here is that wrapping the block in a proxy object will probably introduce edge-case bugs: for example, the proxy object can delegatecall
to the wrapped proc easy enough, but what if the matcher wants to useinstance_eval
? I don't think there's an easy way to delegate that. Also, the matcher may haveProc === actual
checks that would start to fail once we pass the matcher our proxy object.expect
and the matcher does not indicate it expects a block via this new API, we could raise an error. The problem here is that it's all or nothing: we can't use this API as a means to detect a matcher expects a block unless all block matchers (including custom block matchers) have been updated to implement this API. That's problematic. I also don't want to make the matcher protocol even bigger.Any other ideas?
The text was updated successfully, but these errors were encountered: