[WIP] Add more specific variants of contain (#818)

[mkolumb]

Hi,

I have implemented this enhancement.
You can find the API in documentation.md
Because I didn't receive a response, I made the following assumptions:

• Contain should throw always if expected string wouldn't be found (as in the previous version)
• aaa contains aa once, aaaaa contains aa twice

[mkolumb]

If I should change something in assumptions please comment and I will fix it.

[dennisdoomen]

See http://dawehner.github.io/github,/code/review/2017/09/08/emoji-code-review.html for the meaning of the emojis.

[jnyrup]

❌ This is a prime example of why I don't like this augmentation of Contain.
ABCDEF contains XYS 0 times, which is less than 1, but the test fails.

[mkolumb]

You're right, that was my assumption.
I think that you as maintainers should decide about API.

I see following solutions:

1. Leave it as is
2. Replace the current Contain by something like that actual.Should().Contain("Fred").Any
3. Expand Contain in different way, e.g. as you suggested in Add version of StringAssertions.Contain that allows specifying the number of times a substring should occur. #818
4. Introduce new method, but I don't know how to name it.

[jnyrup]

@dennisdoomen Here's my beef with this API.
For inspiration, FakeItEasy has shared their considerations, when they changed their API.

[dennisdoomen]

The problem they were facing had to do with the word repeated. We don't have this issue with this API. It reads very fluently IMHO.

[jnyrup]

@dennisdoomen I agree that it reads fluently, but the result of

"A".Should().Contain("B").AtMost.Once();

would fail and be logically inconsistent with the written assertion.

[dennisdoomen]

Yeah, either we make sure our API does not support that, or we reconsider the API

[jnyrup]

x This is a prime example of why I don't like this augmentation of Contain.
"ABCDEF" contains "XYS" 0 times, which is less than 1, but the test fails.

[mkolumb]

The same as in #962 (comment)

[jnyrup]

With the lessons learned from #925 this PR would break ABI compatibility by changing the return of a public member.

We cannot add

StringContainmentConstraints Contain(string expected, ...)

as another overload, as the return type is not part of the function signature and would cause a conflict with the existing

AndConstraint<StringAssertions> Contain(string, ...)

Here are my thoughts on the API.

• Keep source compatibility
• Keep binary compatibility
• Be fluent and explicit
• Be generalizable to other collection assertions.

From all the examples I could think about, 2.b is the clear winner to me.
It uses method overloading rather than method chaining, but it reads fluently.

---

1. "a should contain b at least 5 times"
   a. a.Should().Contain(b).AtLeast.Times(5)
   b. a.Should().Contain(b).AtLeast(5)
   c. a.Should().Contain(b).AtLeast(5.Times())

All would cause ABI breakage if Contain changes return type.
Breaking change if a.Contain(b).Atmost(1) succeeds for zero occurrences of b, but logically inconsistent if it fails.
1.a has fluency problems, as you can't say "at least times 5"
1.b Implicit was 5 is, see note 1

---

2. "a should contain b 5 times at least"
   a. a.Should().Contain(b, 5, Times.AtLeast)
   b. a.Should().Contain(b, 5.Times().AtLeast)
   * a.Should().Contain(b, Exactly.Once())
   * a.Should().Contain(b, MoreThan.Twice())

2.a is not very pretty if generalized, e.g. a.Should().Contain(4, 5, Times.AtLeast)

---

3. "a should at least contain b 5 times"
   a. a.Should().AtLeast.Contain(b, 5)
   b. a.Should().AtLeast.Contain(b, 5.Times())

Both are polluting Should() with AtLeast, AtMost, Exactly, MoreThan, LessThan
3.a Implict was 5 is, see note 1.

---

4. "a should at least 5 times contain b"
   a. a.Should().AtLeast(5).Contain(b)
   b. a.Should().AtLeast(5.Times()).Contain(b)

Both are polluting Should() with AtLeast, AtMost, Exactly, MoreThan, LessThan
4.a Implict was 5 is, see note 1.

---

note 1: consider e.g. new int[] { 1, 2, 2, 3}.Should().Exactly.Contain(2, 2)
note 2: I could image that other packages have an extension method named Times(int), so this extensions method should probably be put in the FluentAssertions.Extensions namespace as we did in #675

[dennisdoomen]

With the lessons learned from #925 this PR would break ABI compatibility by changing the return of a public member.

What if the return type inherits from the original return type?

[dennisdoomen]

Hmmmm.... https://stackoverflow.com/questions/1456785/a-definitive-guide-to-api-breaking-changes-in-net

[jnyrup]

I just tried and changing the return type to a child class of the original return type breaks binary compatibility.

[dennisdoomen]

So what do we want to do with this PR?

[jnyrup]

I would prefer if this was reusable, such that other Contain* assertions could benefit from it.

E.g.

• CollectionAssertions.ContainEquivalentOf(TExpectation expectation, ...)
• NonGenericCollectionAssertions.Contain(object expected, ...)
• SelfReferencingCollectionAssertions.Contain(T expected, ...)
• SelfReferencingCollectionAssertions.Contain(Expression<Func<T, bool>> predicate, ...)
• StringAssertions.Contain(string expected, ...)
• StringAssertions.ContainEquivalentOf(string expected, ...)
• (GenericDictionaryAssertions.ContainValue(TValue expected, ...))

[dennisdoomen]

I would prefer if this was reusable, such that other Contain* assertions could benefit from it.

That's a different concern, right? As far as I remember, the discussion ended with the breaking nature of this PR. IMO, this is something we should avoid.

[dennisdoomen]

Hmm. Looking through the entire discussion, I feel (1) is still the best API but will cause breaking changes. (2) doesn't have that problem, but may cause naming conflicts with people that are using FakeitEasy as well. I definitely don't like (3) and (4).

[jnyrup]

I would prefer if this was reusable, such that other Contain* assertions could benefit from it.

That's a different concern, right? As far as I remember, the discussion ended with the breaking nature of this PR. IMO, this is something we should avoid.

It's an additional concern.
It would feel weird to me if one overload of Contain got support but not the other.
E.g.

"aaa".Should().Contain("a") // <-- would get support from this PR
new int[] { 1, 1, 1 }.Should().Contain(1) // <-- wouldn't

[jnyrup]

@mkolumb: @dennisdoomen and I have discussed the API and we found that your earlier API proposal ended up being the best non-breaking logically consistent way to implement this feature.
At least from our perspectives.

public delegate bool MatchesOccurenceCount(int actual);

public AndConstraint<StringAssertions> Contain(string expected, MatchesOccurenceCount predicate, string because = "", params object[] becauseArgs)

// Usage
actual.Should().Contain("Fred", AtLeast.Twice, "Fred is important");

// Predefined delegates
{AtLeast, AtMost, MoreThan, LessThan, Exactly}.{Once, Twice, Times(int count)}
(Maybe leave out weird ones such at LessThan.Once?)

// Custom predicates
actual.Should().Contain("Fred", count => count % 2 == 0, "Fred must appear an equal number of times");

[mkolumb]

@mkolumb: @dennisdoomen and I have discussed the API and we found that your earlier API proposal ended up being the best non-breaking logically consistent way to implement this feature.
At least from our perspectives.

Ok, I will change it, probably next weekend.

[dennisdoomen]

Ok, I will change it, probably next weekend.

Sorry for the confusion. But deciding on the trade-off between a nice fluent and intuitive API and backwards compatibility proved to be less than trivial.

[dennisdoomen]

Is this still on your radar?

[mkolumb]

Is this still on your radar?

Yes, sorry, I didn't have a time to finish it. I will do this this week

[jnyrup]

I'm very interested in how good failure messages we can get.

[dennisdoomen]

@mkolumb can we still motivate you to finish this?

[dennisdoomen]

Since there has been no activity for 6 months, let's close it.