Add EqualTo overload with equality comparer

[martinskuta]

Hi, we have started using FakeItEasy in our main code base and while migrating from RhinoMocks I missed a method argument constraint where you can use EqualTo with custom comparer. Originaly we were using That.Matches(Predicate) constraint, then we have created our own extension method for that to be able to pass equality comparer and I thought it would be nice if it was part of the framework so I created this PR if you are interested.

Cheers 🎆

[blairconrad]

Hi, @martinskuta. Thanks for your interest. We'll need to think about this potential API change for a bit, but to make sure I understand, the benefit is that you can use

A.CallTo(() => fake.Method(A<string>.That.IsEqualTo("IgnoreCase", StringComparer.OrdinalIgnoreCase))

instead of

A.CallTo(() => fake.Method(A<string>.That.Matches(s => StringComparer.OrdinalIgnoreCase.Equals(s, "IgnoreCase"))

(or instead of writing the extension method that you mentioned)?

[martinskuta]

Hi, @martinskuta. Thanks for your interest. We'll need to think about this potential API change for a bit, but to make sure I understand, the benefit is that you can use

A.CallTo(() => fake.Method(A<string>.That.IsEqualTo("IgnoreCase", StringComparer.OrdinalIgnoreCase))

instead of

A.CallTo(() => fake.Method(A<string>.That.Matches(s => StringComparer.OrdinalIgnoreCase.Equals(s, "IgnoreCase"))

(or instead of writing the extension method that you mentioned)?

Yes, exactly. To make it more human readable and to have better constraint violation message.

[blairconrad]

Ah, I hadn't thought about the output! I see

FakeItEasyQuestions.Test+TestInterface.SetName(name: <s => StringComparer.OrdinalIgnoreCase.Equals(s, "InsEnsitive")>)

vs.

FakeItEasyQuestions.Test+TestInterface.SetName(name: <equal to "InsEnsitive">)

The second certainly reads nicer. I worry a little bit that there's no indication that non-standard "Equals" was applied, but then again, it won't happen without a conscious choice, so is probably okay.

[blairconrad]

@martinskuta, the more I think about the enhancement, the more I like it. We'll need @thomaslevesque to weigh in, but for now I'll critique the PR as if we're planning on adding the feature. There's a chance @thomaslevesque might not like it, though, so if you'd be sad at responding to my comments and having to drop the work later, you might not want to rush in.

[blairconrad]

Looks generally good, @martinskuta! I have some fairly small requests, though.
In addition to the line-level comment I made elsewhere, I'd note that

1. the API consistency ("approval") tests now fail, because of the new entry point. One easy way to resolve this is to run build.cmd force-approve at the top level of the repo. That will copy the new API definition files into the expected ones. Commit them and the tests should start passing
2. we should have extra documentation so users can discover this overload. As I often say to colleagues "docs or it didn't happen". If you'd be so kind, I think adding a row to the big table in argument-constraints.md would cover it.

[blairconrad]

There's a concern here that comparer will be null. I would Guard.Against it here and also add a test to EqualToWithComparerConstraintTests to ensure that it's null-guarded. You can look for examples of BeNullGuarded elsewhere in the code.

[martinskuta]

✅ Added the null guarding and related test.

[martinskuta]

I had only one concern with the test for guarding against comparer being null, I was not sure if it is the correct you would write such test for it, so have a closer look to that :)

[thomaslevesque]

Thanks @martinskuta!
I think it's a very useful addition

[martinskuta]

The second certainly reads nicer. I worry a little bit that there's no indication that non-standard "Equals" was applied, but then again, it won't happen without a conscious choice, so is probably okay.

Yeah, I was thinking about the message to say something like "is equal to xx using yyy comparer" but then I thought is is just fine with "is equal to" because you consciosly used comparer so you should know what is going on.

Looks generally good, @martinskuta! I have some fairly small requests, though.
In addition to the line-level comment I made elsewhere, I'd note that

Thx for review! I created the PR just to see if there is interest in the feature. I will adapt code according to your comments later tomorrow (or I should say today already :) and then we can move it forward and get it eventually merged :)

Thanks @martinskuta!
I think it's a very useful addition

You're welcome, thank you for awesome framework :)

[martinskuta]

1. the API consistency ("approval") tests now fail, because of the new entry point. One easy way to resolve this is to run build.cmd force-approve at the top level of the repo. That will copy the new API definition files into the expected ones. Commit them and the tests should start passing
2. we should have extra documentation so users can discover this overload. As I often say to colleagues "docs or it didn't happen". If you'd be so kind, I think adding a row to the big table in argument-constraints.md would cover it.

✅ Updated docs (argument-constraints.md) with the new IsEqualTo(object, equalityComparer) overload
✅ Updated API definition file with new IsEqualTo(object, comparer) overload

[martinskuta]

@blairconrad thx for the review. I have addressed all your comments and pushed suggested changes. The build is now passing, so it is now in your hands :)

[blairconrad]

@martinskuta, I think it's perfect. Thank you. I'll let @thomaslevesque take a look, but I think it's mergeable.

[thomaslevesque]

Looks very good to me. Thanks again @martinskuta!

[afakebot]

This change has been released as part of FakeItEasy 7.0.0-beta.1.

[blairconrad]

Thanks, @martinskuta. Look for your name in the release notes! 🥇

[blairconrad]

This change has also been released as part of FakeItEasy 7.0.0.

Thanks again, @martinskuta.