-
Notifications
You must be signed in to change notification settings - Fork 268
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
Update arg matcher misuse docs #549
Conversation
3cbd056
to
3712cc4
Compare
|
||
// OK: Use arg matcher to configure a callback: | ||
var makeCalled = false; | ||
widgetFactory.When(x => x.Make(Arg.Any<int>())).Do(x => makeCalled = true); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code snippet is very nice, but I'm afraid that we might demonstrate people anti-pattern here, as Received()
should be used for this particular case. They might not remember later the exact context, but will refer like "I saw somewhere in the official docs them doing this" (happens to me often 😟 ).
Instead, we might use Do
here for the exactly same purpose as we use Arg.Do()
below:
...Do(x => makeCalledForId_Way1 = x.ArgAt<int>(0));
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very good point, but I think we'd need much larger changes to the docs to get better examples. Even for this we should really do .Received().Make(42)
rather than catching 42
and asserting.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, your concern about Received()
is fully valid.
I got another idea how we could prettify it. What if we change interface to the following:
public class WidgetInfo {
public string Name { get; set; }
}
public interface IWidgetFactory {
object Make(WidgetInfo info);
}
Then we can improve our samples as following:
// NOT OK: using an arg matcher as a value, not to specify a call:
// widgetFactory.Make(Arg.Any<WidgetInfo>()).Returns(Arg.Any<object>());
// Instead use something like:
widgetFactory.Make(Arg.Any<WidgetInfo>()).Returns(new { Name = "Any widget" });
and
var widgetFactory = Substitute.For<IWidgetFactory>();
var subject = new Sprocket(widgetFactory);
// OK: Use arg matcher to configure a callback:
var log = new List<string>();
widgetFactory.When(x => x.Make(Arg.Any<WidgetInfo>()).Do(x => log.Add(x.Arg<WidgetInfo>().Name));
// OK: Use Arg.Do to configure a callback:
var log2 = new List<string>();
widgetFactory.Make(Arg.Do<WidgetInfo>(info => log2.Add(info.Name)));
It would be a perfectly valid scenario, when you would indeed use callbacks instead of asserting that via ugly "Arg.Is()` inspection.
P.S. Are you using some kind of button to fill descriptions like this or it's your new life style? 😅 Now all the PR descriptions looks like there is some nasty "MR. ONE" causing all our troubles 🤣 |
346f5f4
to
9aac5a3
Compare
@zvirja I've updated the example to use a log rather than something that should be using The reason for Mr One is that I was trying to make it like a footnote so the long link doesn't get in the way of otherwise wrapped text, but it renders funny. Oops. 😄 Let me know if the changes look ok. |
0a732f3
to
f753293
Compare
@dtchepak Left you yet another idea. Feel free to accept/reject it whatever you feel suits better and go ahead with a merge 😉 |
31133b9
to
c908313
Compare
Update after discussion at issue 35[^1]. [^1]: nsubstitute/NSubstitute.Analyzers#35 (comment)
c908313
to
3ed3dca
Compare
Update after discussion at issue 35.