Provide `self` (the instance) to #any_instance blocks #175

Closed
zettabyte opened this Issue Aug 16, 2012 · 18 comments

Comments

Projects
None yet
@zettabyte

Feature Request

For both stubs and method expectations on #any_instance, where the user provides a block implementation, it would be great if there were a way to get at the actual instance on which the method actually gets called.

One possible way would be to include the instance as an extra parameter to the block, at the end of the list, if the arity of the block indicates the user's block expects to receive this extra instance parameter (and leave it off otherwise).

@patmaddox

This comment has been minimized.

Show comment
Hide comment
@patmaddox

patmaddox Sep 11, 2012

hrm...I can see how this is useful. I wonder if the instance should go first so you can use *args as the second block param.

Although this brings to mind the discussion in #23 - wonder if some info about the stubbed out object & method should be exposed inside the stub block somehow.

hrm...I can see how this is useful. I wonder if the instance should go first so you can use *args as the second block param.

Although this brings to mind the discussion in #23 - wonder if some info about the stubbed out object & method should be exposed inside the stub block somehow.

@benfyvie

This comment has been minimized.

Show comment
Hide comment
@benfyvie

benfyvie Oct 24, 2012

+1 I really need this functionality and can see how it would be beneficial to others as well

+1 I really need this functionality and can see how it would be beneficial to others as well

@coderanger

This comment has been minimized.

Show comment
Hide comment
@coderanger

coderanger Nov 21, 2012

Obligatory 👍 on this, just spent hours trying to figure out a good way around not having this because Rails filters are evil.

Obligatory 👍 on this, just spent hours trying to figure out a good way around not having this because Rails filters are evil.

@Exoth

This comment has been minimized.

Show comment
Hide comment
@Exoth

Exoth Feb 19, 2013

+1
This functionality would be very useful to me.

Exoth commented Feb 19, 2013

+1
This functionality would be very useful to me.

@JonRowe

This comment has been minimized.

Show comment
Hide comment
@JonRowe

JonRowe Mar 6, 2013

Member

Hey @zettabyte how are you getting along with this?

Member

JonRowe commented Mar 6, 2013

Hey @zettabyte how are you getting along with this?

@zettabyte

This comment has been minimized.

Show comment
Hide comment
@zettabyte

zettabyte Mar 6, 2013

@JonRowe: I'm still all for this feature. Early on after I first reported it I attempted an implementation so I could submit a pull request for consideration. However, I'd need to spend more time (than I've had available) to get it to actually work; I'm completely unfamiliar with rspec's code.

An aside, every time I see the issue title with the word this I'm reminded that I was busy doing a lot of javascript at the time this came up (should be self). :-)

@JonRowe: I'm still all for this feature. Early on after I first reported it I attempted an implementation so I could submit a pull request for consideration. However, I'd need to spend more time (than I've had available) to get it to actually work; I'm completely unfamiliar with rspec's code.

An aside, every time I see the issue title with the word this I'm reminded that I was busy doing a lot of javascript at the time this came up (should be self). :-)

@myronmarston

This comment has been minimized.

Show comment
Hide comment
@myronmarston

myronmarston Mar 6, 2013

Member

I'd still like to get this in; the hardest part is that if we just change to this it'll be a backwards incompatibility, because the args yielded to the block implementation will change. I think that to make this change we'll have to provide a config option that defaults to off, so it reminds 100% backwards compatible.

Member

myronmarston commented Mar 6, 2013

I'd still like to get this in; the hardest part is that if we just change to this it'll be a backwards incompatibility, because the args yielded to the block implementation will change. I think that to make this change we'll have to provide a config option that defaults to off, so it reminds 100% backwards compatible.

@dblock

This comment has been minimized.

Show comment
Hide comment
@dblock

dblock Apr 2, 2013

Contributor

+1

Contributor

dblock commented Apr 2, 2013

+1

@gwintrob

This comment has been minimized.

Show comment
Hide comment
@gwintrob

gwintrob Apr 17, 2013

+1

+1

@arkadiyt

This comment has been minimized.

Show comment
Hide comment
@arkadiyt

arkadiyt Apr 17, 2013

+1

+1

@JonRowe

This comment has been minimized.

Show comment
Hide comment
@JonRowe

JonRowe Apr 19, 2013

Member

@myronmarston what about if we evaluated the blocks in the context of the instance, thus making self the reference to the instance?

Member

JonRowe commented Apr 19, 2013

@myronmarston what about if we evaluated the blocks in the context of the instance, thus making self the reference to the instance?

@myronmarston

This comment has been minimized.

Show comment
Hide comment
@myronmarston

myronmarston Apr 19, 2013

Member

@JonRowe -- there's a long discussion about that possibility in #236; please see that for the existing discussion, and add your thoughts there.

Member

myronmarston commented Apr 19, 2013

@JonRowe -- there's a long discussion about that possibility in #236; please see that for the existing discussion, and add your thoughts there.

@JonRowe

This comment has been minimized.

Show comment
Hide comment
@JonRowe

JonRowe Apr 19, 2013

Member

Ah ok it was considered before ;)

Member

JonRowe commented Apr 19, 2013

Ah ok it was considered before ;)

@timscott

This comment has been minimized.

Show comment
Hide comment
@timscott

timscott Jun 16, 2013

+1

+1

@kdank

This comment has been minimized.

Show comment
Hide comment
@kdank

kdank Jun 19, 2013

+1

kdank commented Jun 19, 2013

+1

@myronmarston

This comment has been minimized.

Show comment
Hide comment
@myronmarston

myronmarston Jun 21, 2013

Member

We're still planning on this for RSpec 3. There's a comment from #236 (which we decided not to merge) that I thought would be useful to quote here so folks know the direction we'll be going with this and the reasons:

Anyhow, you've taken a quite different (but interesting) approach here to what I had in mind. Originally I was thinking it would work like this:

# normally, this is how it works...
MyClass.any_instance.stub(:foo) do |arg_1, arg_2, &block|
end

# but if you change the config...
RSpec::Mocks.configure do |c|
  c.yield_instance_to_any_instance_implementation_blocks = true
end

# ...then the yielded args change to:
MyClass.any_instance.stub(:foo) do |my_class_instance, arg_1, arg_2, &block|
end

I think the implementation of this approach would be much, much simpler: no need to mess with defining a method dynamically, or eval'ing the block in a different context or anything...we just add a simple conditional that checks the value of the config option, and, if it is true, prepends the yield args list with the object instance before calling the implementation block.

On top of that, I think it could be potentially troublesome to change the eval context in the implementation block like that. It's a much bigger change (as you suddenly can't call helper methods defined in the example group like you could before) compared to just yielding an extra arg; plus it's more inline with how block implementations work elsewhere in rspec-mocks.

In my opinion, we should have included the object instance in the yielded args from the very start. We didn't think things through to realize that without this, users have no way to get a reference to the stubbed object. Given that, I'd like to move in the direction of making the object instance always be yielded from the any_instance block implementations. We can change the spec_helper.rb generate by rspec --init so that it turns this on by default for new projects, and we can consider making it default to on in some future major release (e.g. maybe RSpec 3 or 4), while still allowing folks to disable it as needed. Making it a global config is far simpler than adding to the fluent interface, and essentially "corrects" the oversite that caused us to not include the object instance in the first place.

It's true that folks who want to use this feature would need to turn on the config option, and go through and fix all their existing any_instance block implementations, but that doesn't bother me too much...we're providing a new feature that users have to opt-in to use on an existing project, and I would expect it to be pretty easy to use an editor macro to go through a fix all uses of any_instance block implementations in a short time period.

BTW, we briefly discussed a similar addition to the fluent interface to configure what args a particular block implementation is yielded, but decided against it for various reasons. The discussion is here:

#23 (comment)

Member

myronmarston commented Jun 21, 2013

We're still planning on this for RSpec 3. There's a comment from #236 (which we decided not to merge) that I thought would be useful to quote here so folks know the direction we'll be going with this and the reasons:

Anyhow, you've taken a quite different (but interesting) approach here to what I had in mind. Originally I was thinking it would work like this:

# normally, this is how it works...
MyClass.any_instance.stub(:foo) do |arg_1, arg_2, &block|
end

# but if you change the config...
RSpec::Mocks.configure do |c|
  c.yield_instance_to_any_instance_implementation_blocks = true
end

# ...then the yielded args change to:
MyClass.any_instance.stub(:foo) do |my_class_instance, arg_1, arg_2, &block|
end

I think the implementation of this approach would be much, much simpler: no need to mess with defining a method dynamically, or eval'ing the block in a different context or anything...we just add a simple conditional that checks the value of the config option, and, if it is true, prepends the yield args list with the object instance before calling the implementation block.

On top of that, I think it could be potentially troublesome to change the eval context in the implementation block like that. It's a much bigger change (as you suddenly can't call helper methods defined in the example group like you could before) compared to just yielding an extra arg; plus it's more inline with how block implementations work elsewhere in rspec-mocks.

In my opinion, we should have included the object instance in the yielded args from the very start. We didn't think things through to realize that without this, users have no way to get a reference to the stubbed object. Given that, I'd like to move in the direction of making the object instance always be yielded from the any_instance block implementations. We can change the spec_helper.rb generate by rspec --init so that it turns this on by default for new projects, and we can consider making it default to on in some future major release (e.g. maybe RSpec 3 or 4), while still allowing folks to disable it as needed. Making it a global config is far simpler than adding to the fluent interface, and essentially "corrects" the oversite that caused us to not include the object instance in the first place.

It's true that folks who want to use this feature would need to turn on the config option, and go through and fix all their existing any_instance block implementations, but that doesn't bother me too much...we're providing a new feature that users have to opt-in to use on an existing project, and I would expect it to be pretty easy to use an editor macro to go through a fix all uses of any_instance block implementations in a short time period.

BTW, we briefly discussed a similar addition to the fluent interface to configure what args a particular block implementation is yielded, but decided against it for various reasons. The discussion is here:

#23 (comment)

@JonRowe

This comment has been minimized.

Show comment
Hide comment
@JonRowe

JonRowe Jun 21, 2013

Member

Yeah for the record, recent experience has taught me that there's no way we should evaluate the blocks so that self is the instance, providing it to the block as an argument is a far better solution both technically and for developer sanity :)

Member

JonRowe commented Jun 21, 2013

Yeah for the record, recent experience has taught me that there's no way we should evaluate the blocks so that self is the instance, providing it to the block as an argument is a far better solution both technically and for developer sanity :)

@myronmarston

This comment has been minimized.

Show comment
Hide comment
@myronmarston

myronmarston Aug 15, 2013

Member

This has been fixed by @samphippen and will be the default behavior in RSpec 3.

Member

myronmarston commented Aug 15, 2013

This has been fixed by @samphippen and will be the default behavior in RSpec 3.

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