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

BeOneOf for object comparisons with custom comparer support #2112

Closed
xmarshal opened this issue Jan 20, 2023 · 15 comments · Fixed by #2243
Closed

BeOneOf for object comparisons with custom comparer support #2112

xmarshal opened this issue Jan 20, 2023 · 15 comments · Fixed by #2243
Assignees
Labels
api-approved API was approved, it can be implemented

Comments

@xmarshal
Copy link
Contributor

xmarshal commented Jan 20, 2023

Background and motivation

Support for custom comparer to compare objects

API Proposal

public class ObjectAssertions<TSubject, TAssertions> : ReferenceTypeAssertions<TSubject, TAssertions>
    where TAssertions : ObjectAssertions<TSubject, TAssertions>
{
    public AndConstraint<TAssertions> BeOneOf<TExpectation>(IEnumerable<TExpectation> validValues, IEqualityComparer<TExpectation> comparer, string because = "", params object[] becauseArgs);
}

API Usage

var obj = new SomeClass(1);
var expected = new[] { new SomeClass(1), new SomeClass(2), new SomeClass(3) };
obj .Should().BeOneOf(expected, new SomeClassEqualityComparer ());

internal class SomeClass
{
    public SomeClass(int key)
    {
        Key = key;
    }

    public int Key { get; }

    public override string ToString()
    {
        return $"SomeClass({Key})";
    }
}

internal class SomeClassEqualityComparer : IEqualityComparer<SomeClass>
{
    public bool Equals(SomeClass x, SomeClass y)
    {
        if (ReferenceEquals(x, y))
        {
            return true;
        }

        if (ReferenceEquals(x, null))
        {
            return false;
        }

        if (ReferenceEquals(y, null))
        {
            return false;
        }

        if (x.GetType() != y.GetType())
        {
            return false;
        }

        return x.Key == y.Key;
    }

    public int GetHashCode(SomeClass obj)
    {
        return obj.Key;
    }
}

Alternative Designs

No response

Risks

No response

@xmarshal xmarshal added the api-suggestion Early API idea and discussion, it is NOT ready for implementation label Jan 20, 2023
@xmarshal xmarshal changed the title [API Proposal]: BeOneOf for object comparisons with custom comparer support BeOneOf for object comparisons with custom comparer support Jan 20, 2023
@dennisdoomen
Copy link
Member

If we add this to BeOneOf, we must also add this to Be and NotBe for consistency. Assuming @jnyrup agrees, are you willing to include that in your PR @xmarshal ?

@jnyrup
Copy link
Member

jnyrup commented Jan 21, 2023

In the proposed signature validValues can have a different type TExpectation than the subject TSubject.

    public AndConstraint<TAssertions> BeOneOf<TExpectation>(IEnumerable<TExpectation> validValues, IEqualityComparer<TExpectation> comparer, string because = "", params object[] becauseArgs);

With the example usage you provide, re-using TSubject for validValues should suffice.

    public AndConstraint<TAssertions> BeOneOf(IEnumerable<TSubject> validValues, IEqualityComparer<TSubject> comparer, string because = "", params object[] becauseArgs);

Also only BeEquivalentTo in ObjectAssertions currently uses TExpectation.
Can you clarify if this is unintentional or why we should add TExpectation.

For even more consistency, we should also add this to BeOneOf and [Not]Be for ComparableTypeAssertions.

For discoverability, this topic is related to #699.

@xmarshal
Copy link
Contributor Author

xmarshal commented Jan 22, 2023

In the proposed signature validValues can have a different type TExpectation than the subject TSubject.

Mostly obj.Should() extension method returns non-generic ObjectAssertions
So TSubject will be object
May be we should add ObjectAssertions<TSubject> : ObjectAssertions<TSubject, ObjectAssertions<TSubject>> instead of ObjectAssertions : ObjectAssertions<object, ObjectAssertions> and then TSubject can be used for IEnumerable<TSubject> validValues

@xmarshal
Copy link
Contributor Author

If we add this to BeOneOf, we must also add this to Be and NotBe for consistency. Assuming @jnyrup agrees, are you willing to include that in your PR @xmarshal ?

I will add when we reach an agreement for API

@dennisdoomen
Copy link
Member

May be we should add ObjectAssertions : ObjectAssertions<TSubject, ObjectAssertions> instead of ObjectAssertions : ObjectAssertions<object, ObjectAssertions> and then TSubject can be used for IEnumerable validValues

That would require a Should<T> as well. But due to the way overload resolution works, it will overrule every other Should extension method.

@xmarshal
Copy link
Contributor Author

xmarshal commented Jan 22, 2023

That would require a Should<T> as well. But due to the way overload resolution works, it will overrule every other Should extension method.

For even more consistency, we should also add this to BeOneOf and [Not]Be for ComparableTypeAssertions.

So what I should do next?

@dennisdoomen
Copy link
Member

Nothing. I'm waiting for @jnyrup's opinion.

@jnyrup
Copy link
Member

jnyrup commented Jan 22, 2023

An alternative API

public class ObjectAssertions : ObjectAssertions<object, ObjectAssertions>
{
    public AndConstraint<ObjectAssertions> BeOneOf<TExpectation>(IEnumerable<TExpectation> validValues, IEqualityComparer<TExpectation> comparer, string because = "", params object[] becauseArgs);
}

public class ObjectAssertions<TSubject, TAssertions> : ReferenceTypeAssertions<TSubject, TAssertions>
    where TAssertions : ObjectAssertions<TSubject, TAssertions>
{
    public AndConstraint<TAssertions> BeOneOf(IEnumerable<TSubject> validValues, IEqualityComparer<TSubject> comparer, string because = "", params object[] becauseArgs);
}

The difference comes for classes deriving from ObjectAssertions<TSubject, TAssertions> e.g. HttpResponseMessageAssertions.
The proposed API would allow HttpResponseMessageAssertions.BeOneOf() to take input of arbitrary types, while it currently only allows input of HttpResponseMessage.

@xmarshal
Copy link
Contributor Author

alternative API added
should I add Be and NotBe and what signature You suggest?

@dennisdoomen
Copy link
Member

An alternative API

Looks good to me.

@xmarshal
Copy link
Contributor Author

Be and NotBe with comparer added

@jnyrup jnyrup added the api-approved API was approved, it can be implemented label Jan 24, 2023
@jnyrup
Copy link
Member

jnyrup commented Jan 24, 2023

The "alternative API" together with Be and NotBe is approved 👍

@xmarshal
Copy link
Contributor Author

ok will add test soon

@Corniel
Copy link
Contributor

Corniel commented May 15, 2023

If TSubject imlements IEquatable<TSubject> I would hope that providing IEqualityComparer<TSubject> is optional?

@jnyrup
Copy link
Member

jnyrup commented May 19, 2023

If TSubject imlements IEquatable<TSubject> I would hope that providing IEqualityComparer<TSubject> is optional?

If not providing an IEqualityComparer, wouldn't the existing BeOneOf suffice?
See this example: https://dotnetfiddle.net/N2Rpt6

@dennisdoomen dennisdoomen removed the api-suggestion Early API idea and discussion, it is NOT ready for implementation label May 28, 2023
@jnyrup jnyrup linked a pull request Jul 30, 2023 that will close this issue
7 tasks
@jnyrup jnyrup closed this as completed Jul 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api-approved API was approved, it can be implemented
Projects
None yet
4 participants