-
Notifications
You must be signed in to change notification settings - Fork 731
Adds SelfReferencingCollectionAssertions.Satisfy method which is not sensitive to the order of elements and accepts predicate expressions. #1500
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
Conversation
|
That's an impressive piece of work. Thanks for that. |
jnyrup
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Really nice work!
Only minor comments from here.
Src/FluentAssertions/Collections/SelfReferencingCollectionAssertions.cs
Outdated
Show resolved
Hide resolved
Tests/FluentAssertions.Specs/Collections/GenericCollectionAssertionsSpecs.cs
Outdated
Show resolved
Hide resolved
Tests/FluentAssertions.Specs/Collections/GenericCollectionAssertionsSpecs.cs
Outdated
Show resolved
Hide resolved
Src/FluentAssertions/Collections/SelfReferencingCollectionAssertions.cs
Outdated
Show resolved
Hide resolved
Src/FluentAssertions/Collections/SelfReferencingCollectionAssertions.cs
Outdated
Show resolved
Hide resolved
Src/FluentAssertions/Formatting/PredicateLambdaExpressionValueFormatter.cs
Outdated
Show resolved
Hide resolved
Src/FluentAssertions/Formatting/PredicateLambdaExpressionValueFormatter.cs
Outdated
Show resolved
Hide resolved
Src/FluentAssertions/Formatting/PredicateLambdaExpressionValueFormatter.cs
Outdated
Show resolved
Hide resolved
|
Dennis, Jonas, thanks a lot for your support! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
**Comments
- 👍 Impressive work. You've kind of changed my mind about the importance of algorithms,
- 🔧 I think it reads better if you replace the term
NotMatchedwithUnmatchedeverywhere. - 🔧 Since the namespace is already called
MaximumMatching, I would repeat that word on every class and method. - 🔧 I think the code will be much easier to understand if you would wrap the predicate and its index in a first-class class such as
Predicate, and then have it overrideEquals. This will reduce the amount of fiddling around with indexes and using those to find the actual predicate. And this probably also applies to the element collection and its indexes.
Legend for the emojis
• ❓ I have a concrete question. There is nothing here to be changed yet.
• ❌ Here is an actual problem. Please change this according to my suggestion. It might be a bug or a strict code style fix.
• 🔧 Here is an idea/suggestion. I think it would improve things, but feel free to disagree :)
• 🙃 This is a nitpick. Feel free to change of leave it.
• 🤔 This might be wrong. Let’s talk about it, maybe we can come up with a better solution
• 🤡 Here is a problem independent from your work. It would be nice if you would be willing to include it
Src/FluentAssertions/Collections/MaximumMatching/MaximumMatchingProblem.cs
Outdated
Show resolved
Hide resolved
Src/FluentAssertions/Collections/MaximumMatching/MaximumMatchingProblem.cs
Outdated
Show resolved
Hide resolved
Src/FluentAssertions/Collections/MaximumMatching/MaximumMatchingProblem.cs
Show resolved
Hide resolved
Src/FluentAssertions/Collections/MaximumMatching/MaximumMatchingProblem.cs
Outdated
Show resolved
Hide resolved
Src/FluentAssertions/Collections/MaximumMatching/MaximumMatchingProblem.cs
Outdated
Show resolved
Hide resolved
Src/FluentAssertions/Formatting/PredicateLambdaExpressionValueFormatter.cs
Outdated
Show resolved
Hide resolved
Tests/FluentAssertions.Specs/Collections/GenericCollectionAssertionsSpecs.cs
Outdated
Show resolved
Hide resolved
Tests/FluentAssertions.Specs/Collections/GenericCollectionAssertionsSpecs.cs
Outdated
Show resolved
Hide resolved
Tests/FluentAssertions.Specs/Collections/GenericCollectionAssertionsSpecs.cs
Outdated
Show resolved
Hide resolved
A middle way could maybe be to make two small structs to wrap the raw struct PredicateIndex : IEquatable<PredicateIndex>
{
private int value;
...
}
struct ElementIndex : IEquatable<ElementIndex>
{
private int value;
...
}Then private readonly Dictionary<int, int> matchedElementIndexesByPredicateIndex;could be private readonly Dictionary<PredicateIndex, ElementIndex> matchedElementIndexes; |
|
Pardon. I didn't realize that I was pushing iterations without squashing my local commits. I'm used to working with Azure Dev Ops rather than Git Hub and it does squashing automatically. |
Tests/Approval.Tests/ApprovedApi/FluentAssertions/net47.approved.txt
Outdated
Show resolved
Hide resolved
Src/FluentAssertions/Formatting/PredicateLambdaExpressionValueFormatter.cs
Outdated
Show resolved
Hide resolved
Src/FluentAssertions/Collections/MaximumMatching/MaximumMatchingSolver.cs
Outdated
Show resolved
Hide resolved
…bedev/fluentassertions into ilebedev/ExactlyContain
Introduce a version of the
SelfReferencingCollectionAssertions.SatisfyRespecitvelymethod which is not sensitive to the order of elements and accepts predicate expressions.Example of an assertion:
Example of a failure message:
The discussion about this API is tracked here:
#1480
To enable the necessary functionality three major changes were introduced:
SatisfyAPI was added to theSelfReferencingCollectionAssertionsclass.MaximumMatchingSolverclass was added to encapsulate the simplified version of the Ford-Fulkerson algorithm to find matching between the asserted predicates and the elements of the collection.PredicateLambdaExpressionValueFormatterclass was introduced to produce more human-readable descriptions of predicates in assertion failure messages.IMPORTANT
AcceptApiChanges.ps1/AcceptApiChanges.sh.