Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Proxy blocks along with arguments in has matchers #429

Merged
merged 1 commit into from Jan 19, 2014

Conversation

Projects
None yet
2 participants
Contributor

dgalarza commented Jan 17, 2014

Currently, when using the has matcher any arguments passed are sent
along to the method being proxied. However, any blocks that were provided were
being lost. This commit sends along any blocks with the arguments.

<@dgalarza | @hrs | @masonforest>

Owner

myronmarston commented Jan 18, 2014

Thanks for the PR! I agree the block should be sent along, but I question if it should be included in the failure message, since block inspect output is basically gibberish:

irb(main):001:0> def foo(&b)
irb(main):002:1> puts b.inspect
irb(main):003:1> end
=> nil
irb(main):004:0> foo { }
#<Proc:0x007f9a739cb318@(irb):4>

What do you think about forwarding the block but not including it in the failure message?

Contributor

dgalarza commented Jan 18, 2014

I don't see a problem leaving out the block from the failure message. I agree it doesn't really display anything very useful there, and probably just obfuscates the error message a bit. One of the first reasons we ended up adding it in was to add a test for verifying the block was being passed along, this seemed like the easiest place to add it, similar to how the arguments appear to be tested.

Do you think we need a test for covering blocks are passed along? Or just drop the block from the failure message and move on without it?

Owner

myronmarston commented Jan 18, 2014

Do you think we need a test for covering blocks are passed along? Or just drop the block from the failure message and move on without it?

Yes, since it's a change in behavior it needs specs. We rely heavily on our test suite. My suggestion is to define an object with a has_xyz method that accepts a block and uses the block to determine if it should return true or false. Then you can have a passing and a failing example that use different blocks, to demonstrate that the block is forwarded and used properly.

Contributor

dgalarza commented Jan 18, 2014

Sounds good, thanks for pointing me in the right direction.

Proxy blocks along with arguments in has matchers
Currently, when using the has matcher any arguments passed are sent
along to the method being proxied. However, any blocks that were provided were
being lost. This commit sends along any blocks with the arguments.

<@dgalarza | @hrs | @masonforest>

myronmarston added a commit that referenced this pull request Jan 19, 2014

Merge pull request #429 from dgalarza/dg-hrs-proxy-block-with-has
Proxy blocks along with arguments in has matchers

@myronmarston myronmarston merged commit 4552c14 into rspec:master Jan 19, 2014

1 check passed

default The Travis CI build passed
Details

myronmarston added a commit that referenced this pull request Jan 19, 2014

A few post-merge fixups for #429.
- Add changelog.
- Combine the two specs into one -- I consider it to
  be one behavior (e.g. it forwards the block).
- Allow a `do...end` block as well, and specify what
  happens when both block forms are used.
Owner

myronmarston commented Jan 19, 2014

Thanks, @dgalarza! I merged and then applied a few minor improvements in 9cf314e. Let me know if you have any questions.

Contributor

dgalarza commented Jan 20, 2014

Awesome, thanks @myronmarston!

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