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

Allow using an IEqualityComparer in BeEquivalentTo #1236

Closed
dennisdoomen opened this issue Jan 14, 2020 · 7 comments · Fixed by #1284
Closed

Allow using an IEqualityComparer in BeEquivalentTo #1236

dennisdoomen opened this issue Jan 14, 2020 · 7 comments · Fixed by #1284
Assignees

Comments

@dennisdoomen
Copy link
Member

dennisdoomen commented Jan 14, 2020

Actually, regarding my suggestion, it would also work if you could supply an EqualityComparer for EquivalencyAssertionOptions, or is it already possible? I'm just saying because EqualityComparer is the "standard" .NET way of doing it, and often there is already a suitable equality comparer available that could be reused as is. At least in our case, I like writing EqualityComparers because they nicely encapsulate the equivalency. I don't like repeating equivalency rules in-line, because usually the same rules are needed in many places. I guess you could encapsulate them as IEquivalencySteps, but why do that if you already have EqualityComparer?

For example

orderDto.Should().BeEquivalentTo(order, options => options
    .Using<DateTimeEqualityComparer>());

or

orderDto.Should().BeEquivalentTo(order, options => options
    .Using<DateTime>(new DateTimeEqualityComparer()));
@jnyrup
Copy link
Member

jnyrup commented Jan 15, 2020

I like the second one the most.
It's explicit and generic.

public static void Using<T, TComparer>()
    where TComparer : IEqualityComparer<T>
{
}

or

public static void Using<T>(IEqualityComparer<T> comparer)
{
}

I'm not sure what the signature of the first one would be?

public static void Using<TComparer>()
    where TComparer : IEqualityComparer
{
}

I dislike that as:

  • It's non-generic in the type being compared, I often (if not always) only implement the generic IEqualityComparer<T> and not the non-generic IEqualityComparer
  • It not visible from the test which types are being compared using the comparer.

In general I'd like to take Fluent Assertions to a more generic world.

@codewisdom
Copy link

I really like this idea! Agree with @jnyrup that the second is more generic and clearer. I've been struggling with comparing types in a homegrown library to C# types. Using the DifferentObjectsEquivalencyStep approach I found on SO has been successful up to a point.

@dennisdoomen
Copy link
Member Author

We should support both passing a generic type as well as a generic instance. So two overloads.

@quixoticaxis
Copy link

quixoticaxis commented Feb 13, 2020

public static void Using<T, TComparer>()
    where TComparer : IEqualityComparer<T>, new()

This should probably have a new() constraint, otherwise it's not obvious how the implementation should utilize it.

It would be great to have an option to use a custom comparer with Fluent Assertions. Currently testing generic collection-like classes that accept custom comparers is a bit tedious.

@quixoticaxis
Copy link

Do you think that other methods would benefit from custom comparers that override the default behavior? For example, I found this issue while looking for a way to provide custom comparer for Be and NotBe methods. From the get go, I've found #1247, #1240 right on the first page which both can be solved by enabling custom comparers.

@dennisdoomen dennisdoomen self-assigned this Feb 19, 2020
@dennisdoomen
Copy link
Member Author

Do you think that other methods would benefit from custom comparers that override the default behavior?

Maybe, but that would require separate issues. This is about the BeEquivalentTo API.

@aroques
Copy link

aroques commented Jun 5, 2023

Can this feature be documented in the documentation somewhere? I didn't see it anywhere

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants