Skip to content
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

Allow chaining a callback method before the Returns call. #98

Closed
haacked opened this issue Jan 13, 2013 · 9 comments
Closed

Allow chaining a callback method before the Returns call. #98

haacked opened this issue Jan 13, 2013 · 9 comments
Assignees

Comments

@haacked
Copy link

haacked commented Jan 13, 2013

Suppose I have the following interface.

public interface IDoStuffs
{
    int Calculate();
}

I love the current syntax for setting up a return value. So simple.

var stuffDoer = Substitute.For<IDoStuffs>();
stuffDoer.Calculate().Returns(42);

But if I need to also run a callback, my tight clean code gets a bit ugly. Not too ugly, but ugly. I can no longer use a simple lambda, I have to use a code block.

var manualResetEvent = new ManualResetEvent();
var stuffDoer = Substitute.For<IDoStuffs>();
stuffDoer.Calculate().Returns(x => {
    manualResetEvent.Signal();
    return 42;
});

What I'd love to do instead is chain a callback before the Returns call.

var manualResetEvent = new ManualResetEvent();
var stuffDoer = Substitute.For<IDoStuffs>();
stuffDoer.Calculate().Do(() => manualResetEvent.Signal()).Returns(42);

If it feels more natural, you could chain it to the end instead. But the behavior would probably be the same.

var manualResetEvent = new ManualResetEvent();
var stuffDoer = Substitute.For<IDoStuffs>();
stuffDoer.Calculate().Returns(42).Do(() => manualResetEvent.Signal());

Bonus if the callback method has 2-overloads:

  1. One that accepts a parameterless Action.
  2. One that accepts an Action with the same arguments as the method being substituted.

Thoughts?

@dtchepak
Copy link
Member

Thanks a lot for the suggestion. I think I'd prefer chaining it off .Returns(). We already bloat the methods available on Object, so I'd prefer not to add another extension method to the mix unless doing so makes it much clearer.

I don't mind the code block, but I don't think it would do any harm having Returns() return something that lets you add a callback. Not sure on naming, what about this?

stuffDoer.Calculate().Returns(42).AndCalls(x => blah());

I'll spike it out and see how it goes.

Is it something you need often enough to make it worth expanding the surface area of the API? The code block does have a bit more punctuation noise, but it also has the benefit of not needing to learn more API or read any more docs.

@haacked
Copy link
Author

haacked commented Jan 13, 2013

Chaining it off Returns makes sense.

It does come up quite often for me in async scenarios. Also, it'd make it easier to port code written in other mock frameworks over.

One nice thing about making Returns return something useful is I can write my own extension methods as the need arises.

In fact, you could write this new method as an extension method and hide it in an "Advanced" (or somesuch) namespace so you're main API isn't cluttered.

@ghost ghost assigned dtchepak Feb 3, 2013
@dtchepak
Copy link
Member

I've been spiking this out and wanted to get some opinions on how this should work with the idea of ignoring args:

stuffDoer.Calculate(something).Do(x => { }).Returns(42);
stuffDoer.Calculate(something).DoWithAnyArgs(x => { }).Returns(42);
stuffDoer.Calculate(something).Do(x => { }).ReturnsForAnyArgs(42);
stuffDoer.Calculate(something).DoWithAnyArgs(x => { }).ReturnsForAnyArgs(42);

Should the initial call specification (i.e. Do() or DoWithAnyArgs()) affect what Returns() will match? Or should they be independent?

@haacked
Copy link
Author

haacked commented Feb 21, 2013

Hmm, I thought you were going to chain this off of Returns 😄 #issuecomment-12192640

@dtchepak
Copy link
Member

@haacked, yes, but after discussing it with @AnthonyEgerton we thought sub.Calc().Does(...).Returns(...) would read better. The ambiguities this introduces don't seem to be worth the more natural readability, so looks like we're back to chaining off Returns. :)

@haacked
Copy link
Author

haacked commented Feb 21, 2013

Cool! In that case I assume that you could just have Do that only gets called if Returns actually matches and is always called for ReturnsForAnyArgs.

@dtchepak
Copy link
Member

I've pushed up a first go at this feature @04b0786f70f11a84e385955e80404996a5ece3ac.
It's available in the 1.5.0.6 build on the CI server.

Currently it works like this:

sub.Calculate().Returns(42).AndDoes(x => { /* stuff */ });

Arguments to the call are accessible from x (a CallInfo; there's some info about this in the docs). There are some other examples in the commit.

At the moment I've used AndDoes for the method name to try and match the When..Do and Arg.Do terminology, but I'm not 100% sold on it.

I'd like to experiment with it a bit more before pushing out a release, so please share any feedback you have while it's still easy to tweak the feature without introducing breaking changes. :)

@haacked
Copy link
Author

haacked commented Feb 27, 2013

Cool. I liked Do because it matches the Rx operator, but I'm cool with AndDoes.

@dtchepak
Copy link
Member

Released in v1.6.0.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants