Repeated class is misleading

[jholovacs]

A verification of
IReturnValueArgumentValidationConfiguration<T>.MustHaveHappened(Repeated.Exactly.Once)
in the English language, this means that the call must have happened twice; once, then repeated exactly once.
Recommend changing the name of this Repeated class to Executed or something so there's no confusion.

[thomaslevesque]

It never really bothered me, but I see how it can be confusing...

Another option would be something like MustHaveHappened(Exactly.Once), MustHaveHappened(AtLeast.Twice), etc. It reads nicely, but is not easily discoverable. Interestingly, you can already use this syntax in C# 6 by adding a using static FakeItEasy.Repeated; to the file.

Anyway, changing this would be a breaking change; even though we're not strictly against introducing breaking changes, we usually try to avoid the ones that would affect many users.

@FakeItEasy/owners how do you feel about this?

[blairconrad]

Hi, @jholovacs. Thanks for your interest.

As a native speaker of English, and raging pedant, I agree that it's not technically correct, and that if we were implementing the functionality afresh, a different name would probably be better.

On the other hand the interpretation could be a little bit in the ear of the beholder, and I doubt many people interpret Repeated.Exactly.Once as "happened two times in total".
And one can find many examples (on programming Q&A sites, no less) of people referring to the number of times an action was repeated when they mean how many times an action occurred.

As @thomaslevesque says, the change would be a breaking one. I'm sure there are scores of people happily using the class as it is, who would all of a sudden have to rewrite their tests for what seems like minimal gain to me.

And Mr. Levesque's suggestion of a static using sounds like it will enable you to have a nicer syntax for your own projects, if you're so inclined. If C# 6 isn't available (or even if it is!), you could create the syntax you proposed initially instead:

public class Executed
{
   public static IRepeatSpecification Exactly { get { return Repeated.Exactly; } }
}

public interface IFoo
{
   void FooIt();
}

void Main()
{
   var fake = A.Fake<IFoo>();

   A.CallTo(() => fake.FooIt()).MustHaveHappened(Executed.Exactly.Once);
}

Or you could even add an extension method to enable this syntax:

A.CallTo(() => fake.Add(A<object>._))
    .MustHaveHappened(2.Times());

The possibilities are many.

[adamralph]

This conversation was had before (I'm afraid I can't remember where, it may have been JabbR). IIRC the conclusion was along the lines of "yes it's not perfect", "no it's not worth changing".

That said, we could backlog this as a breaking change and consider it if/when we are planning a new major.

Whilst it reads beautifully, I'd be against adding extension methods to primitive types to allow 2.Times(). IIRC another suggestion was Times.Once, Times.Twice,...

[adamralph]

It could also be done in a minor release if we deprecated Repeated.

[blairconrad]

Whilst it reads beautifully, I'd be against adding extension methods to primitive types to allow

Oh, yeah. That's for end users who really like it and don't mind the tradeoff of the extension method on int. Not for inclusion in FIE.

Although I missed another option in that thread:

A.CallTo(() => fake.Add(A<object>._))
    .MustHaveHappened(3).Times();

which doesn't require an extension on int, only on IAssertConfiguration, which is potentially palatable in the product, except that I don't love the fact that someone might write .MustHaveHappened(3), omit the .Times() and then the condition is never applied. A Roslyn diagnostic would help a little…

If anyone's interested, both extension methods that I discuss can be found at https://gist.github.com/blairconrad/c83f0e6756879d6c8f25 (although they're 2 years old, so may need freshening)

It could also be done in a minor release if we deprecated Repeated.

Adding Executed? I suppose. Technically it could be added even if we didn't deprecate Repeated. But I'd rather change the way of doing things than make two ways. But still I'd rather not replace Repeated with Executed.

If we were going to change the syntax at all, I'd push to look at seeing how we could unify this repeat specification with those of Limited Call Specifications. Not that I'm saying we should. Just if we wanted to make a change at all.

[jholovacs]

I personally like the idea of deprecating Repeated, but I admit I'm biased. I could also see having Executed as an alias/subclass of Repeated. Both of these seem practical to me, not breaking anything yet making more sense from a readability perspective.

[blairconrad]

All:

I continue to think that it's not worth making a change unless we bring more benefits than originally proposed.
The risk of confusing a bunch of probably content users who understand how Repeated is used doesn't justify the change to me.

I've been thinking about the two kinds of "repeat" that we have, and here's what I've come up with:

First, MustHaveHappened. We have a few kinds of repeats:

• Like (expression)
• Never
• AtLeast
• NoMoreThan
• Exactly

The last 3 can all have these tacked on the end:

• Once
• Twice
• Times(n)

For limited calls specs, we just have

• NumberOfTimes(n)

Some examples:

A.CallTo(()=>f.Boo()).MustHaveHappened(Repeated.Like(n => n % 2 == 0));
A.CallTo(()=>f.Boo()).MustHaveHappened(Repeated.Exactly.Once);
A.CallTo(()=>f.Boo()).MustHaveHappened(Repeated.NoMoreThan.Once);
A.CallTo(()=>f.Boo()).MustHaveHappened(Repeated.AtLeast.Once);
A.CallTo(()=>f.Boo()).MustHaveHappened(Repeated.Exactly.Times(3));

A.CallTo(()=>f.Boo()).Returns("yeek").NumberOfTimes(3);

Immediate thoughts, aside from the amibguity of Repeated:

• AtLeast and NoMoreThan are not symmetric. AtMost is probably a better choice
• NumberOfTimes not only doesn't read well, it's a completely different tack on specifying the number of repeats.

An alternative to NumberOfTimes could be

A.CallTo(()=>f.Boo()).Returns("yeek").AtMost(3).Times;

And for the MustHaveHappeneds, maybe something like this

A.CallTo(()=>f.Boo()).MustHaveHappened().ANumberOfTimesMatching(n => n % 2 == 0)); 
A.CallTo(()=>f.Boo()).MustHaveHappened().AtLeast().Once; // AtLeastOnce? 
A.CallTo(()=>f.Boo()).MustHaveHappened().AtLeast(3).Times; 
A.CallTo(()=>f.Boo()).MustHaveHappened().AtMost(3).Times;
A.CallTo(()=>f.Boo()).MustHaveHappened().Exactly().Once; // ExactlyOnce?
A.CallTo(()=>f.Boo()).MustHaveHappened().Exactly(3).Times;

We probably want to keep MustHaveHappened() to be the equivalent of MustHaveHappened().AtLeast(1).Times(), since I bet it's very popular. (Similarly for MustNotHaveHappened, but that's easier.)
That would require us to make the results of MustHaveHappened() mutable, and then we'd probably end up having to put a guard on the implementation to make sure it wasn't mutated twice, like we did with the results of A.CallTo. These are implementation details that should be completely transparent to someone using the fluent API, but I figured I'd bring them up as the mutable object is slightly unsavoury.

Oh. I should mention that any MustHaveHappened() syntax change would have to work with the ordered assertions, so to take the example above, we might end up with something like

A.CallTo(() => fake.Bar(1)).MustHaveHappened().Exactly(3).Times
    .Then(A.CallTo(() => fake.Bar(2)).MustHaveHappened();

Which may complicate the implementation a bit. I suppose the same could be said of @thomaslevesque's #804. I'm still betting on an achievable interface, but
I've not yet spiked an implementation, just thought about it. (Except for A.CallTo(()=>f.Boo()).Returns("yeek").AtMost(3).Times;, which I implemented on a lark.)

I'm keen to hear your overall thoughts—about my resistance to the Repeated change in isolation, about the potential changes to the overall repetition syntax, about whatever.

Thanks.

[adamralph]

Defaulting to P2

[jholovacs]

The .MustHaveHappened().[TimesClause]([Number]) seems like a good idea to me. Discovery through Intellisense, follows the natural language pattern.

[adamralph]

Overall, this seems like a lot of work, and risk, for little gain.

I'm removing breaking from the issue, since it is only a discussion, and a discussion alone cannot break the API. If anything does come out of this, I think we should spin off specific enhancement issues, which may or may not be breaking.

[blairconrad]

Overall, this seems like a lot of work, and risk, for little gain.

I agree. Me, I'd keep everything as is.

[thomaslevesque]

Overall, this seems like a lot of work, and risk, for little gain.

👍

[adamralph]

It seems all the @FakeItEasy/owners feel the same way, so I'm closing the issue.

@jholovacs thanks for raising this and initiating the discussion. I hope you understand why we're closing the discussion. If you feel strongly that we are making the wrong decision here, please feel free to speak up.

[jholovacs]

Well I still think it's confusing, and the reason I brought it up is
because I couldn't figure out why my tests weren't working. If I was
confused, others will be as well. But... if everyone else doesn't want to
do anything about it, oh well.

On Aug 19, 2016 12:00 PM, "Adam Ralph" notifications@github.com wrote:

It seems all the @FakeItEasy/owners
https://github.com/orgs/FakeItEasy/teams/owners feel the same way, so
I'm closing the issue.

@jholovacs https://github.com/jholovacs thanks for raising this and
initiating the discussion. I hope you understand why we're closing the
discussion. If you feel strongly that we are making the wrong decision
here, please feel free to speak up.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#808 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/ABr4dEO2eUmSJT8v2CYbbKihb9BTxKdAks5qhdMZgaJpZM4JXKBY
.

[adamralph]

I guess we could still consider a direct replacement for Repeatedwhich doesn't change the return values of anything. I.e. we discard the proposal of chaining calls onto MustHaveHappened() since that seems quite risky, laborious and breaking, but we still consider deprecated Repeated in favour of Executed or something even better.

@FakeItEasy/owners thoughts?

[thomaslevesque]

but we still consider deprecated Repeated in favour of Executed or something even better.

I don't mind the idea, but Executed doesn't sound very nice. I'd prefer something that AtLeast.Once or Exactly.Twice (without the Repeated.). It could probably be achieved without major changes.

[blairconrad]

If we're interested in making a change for this issue, we should consider moving now that we're (I hope) nearing the end of the 3.0.0 pre-release phase. If we're not, I think we should close it. And I think we should close it.

AtLeast and Exactly without the Repeated prefix sound nice, but at the cost of additional entry points, so that's a bit of a downside. And the effect could be achieved for those who want it via a using static FakeItEasy.Repeated.

Like @thomaslevesque, I don't like the sound of Executed, and I am at a loss for something better.
And even if we found something better, I don't think the benefit we'd get from changing Repeated to another word is worth it; it's been like that for years, and as far as I know there are very few incidents where there's confusion (sorry @jholovacs). I do like it when the code guides the user into correct usage, but I don't think there's a real problem here. Replacing one word for another is not likely to make a big difference, and in this case, the docs are clear (both XML docs on the members and on readthedocs).

[thomaslevesque]

I don't think we should change anything. Repeated isn't perfect, but it's what we have, most users seem happy with it, and we don't have a good alternative.

[herebebeasties]

I hate to rehash a ticket that's been commented on and has a decision made on it, but from where I'm sitting, that Repeated.Once actually means "not repeated at all" looks more like a comedy bug in FakeItEasy than a good design decision.

As committers, perhaps you have a biased viewpoint of API usability issues like this as you're used to the wrinkles in the API? For what it's worth in a survey of 7 random devs at my firm who use FakeItEasy, literally all of them think this is confusing and should be fixed.

How about the alternative of NumberOfTimes - that could be a drop-in replacement that users could easily search + replace to update things. Including that in the framework and encouraging people to use it instead (if not actually initially marking the Repeated API as [Obsolete("Use NumberOfTimes instead")]) would be nicer, I think. (Yes, I know I'm free to implement that myself, but people do tend to follow the "standard" way to do things and it's nice to have people fall into the pit of success.)

[blairconrad]

@herebebeasties, thanks for the comment. We understand your concerns and appreciate having such a passionate community. Sadly, I don't think there's anything new in your argument: changing the API would cause an inconvenience for many many existing users, and the proposed alternative just isn't compelling enough: MustHaveHappened(NumberOfTimes .Exactly.Once) just doesn't flow well enough.

This doesn't mean that we're against any change; we're just not willing to make a sweeping change for what we perceive to be sufficient benefit.
As a bridge, we have created #1288 to update the documentation and samples so that users are pushed toward a less contentious syntax.

[blairconrad]

@thomaslevesque points out that this makes no sense:

This doesn't mean that we're against any change; we're just not willing to make a sweeping change for what we perceive to be sufficient benefit.

of course, I meant

This doesn't mean that we're against any change; we're just not willing to make a sweeping change for what we perceive to be less than sufficient benefit.

[blairconrad]

@thomaslevesque and I went off and chatted about possible replacements for Repeated. Here's an API we're considering:

MustHaveHappened()    // as today
MustNotHaveHappened() // as today

MustHaveHappenedOnceExactly()
MustHaveHappenedTwiceExactly()
MustHaveHappened(3, Times.Exactly)
MustHaveHappened(3, Times.OrMore)
MustHaveHappened(3, Times.OrLess)

MustHaveHappenedANumberOfTimesMatching(n => n %2 == 0)
MustHaveHappenedANumberOfTimesMatching(IsPrime) // where `IsPrime` is a method group

Times would be a new static class.

We like this approach because

1. MustHaveHappened() and MustNotHaveHappened() stay, and they should be by far the most popular of these kinds of methods in use today
2. It eliminates (I think) the confusion over the exact semantics of Repeated
3. It improves discoverability, even over the existing Repeated mechanism
4. We don't have to invert the natural order of words (e.g. Times(3)), nor add an extension method to int

And we can also supply a soft transition: the Repeated variants can live for a major release or two, likely deprecated in 5.0.0 and removed in 6.0.0. In the interim, the documentation can be updated to push users to the new format, either ignoring the old one or explicitly mentioning Repeated as a legacy approach.

Accordingly, I'm reopening the issue. Comments welcome.

[blairconrad]

MustHaveHappenedANumberOfTimesMatching(IsPrime) // where IsPrime is a method group

I'd drop this. It's not supported now, and wouldn't produce good failure messages anyhow. If a call to a helper predicate is needed, we should rely on

MustHaveHappenedANumberOfTimesMatching(n => IsPrime(n))

[thomaslevesque]

Makes sense to me!

[blairconrad]

There's been no objection in nearly a week, so as suggested above, I've created #1292 to track the actual software change.