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

Major refactoring of the EquivalencyValidator #1539

Conversation

dennisdoomen
Copy link
Member

@dennisdoomen dennisdoomen commented May 1, 2021

After a master-class on maintainable code I recently gave, I realized there's still a lot to improve on the equivalency validation code.

  • Renamed IEquivalencyValidator to a more role-based named
  • Encapsulated isComplexTypeMap into ComplexTypeMap
  • Reduce usage of AssertionScope.Current in EquivalencyValidator
  • Attach CyclicReferenceDetector to the EquivalencyValidationContext instead of the current AssertionScope and expose IsCycleReference through IEquivalencyValidationContext
  • Removed CanHandle from IEquivalencyStep
  • Changed the return value of IEquivalencyStep.Handle into an enum with values ContinueWithNext and AssertionCompleted
  • Split-off the subject and expectation from IEquivalencyValidationContext into Comparands to emphasize what we're comparing
  • Renamed EquivalencyStepCollection to EquivalencyPlan as well as the corresponding AssertionOptions property
  • Added the typed base-class EquivalencyStep<T> for convenience
  • Updated the extensibility documentation as well as the upgrade section

Fixes #1534

@dennisdoomen
Copy link
Member Author

dennisdoomen commented May 1, 2021

@jnyrup thoughts on this (before I go to far)?

Comment on lines 3 to 6
public interface IValidateNestedEquivalency
{
void RecursivelyAssertEquality(IEquivalencyValidationContext context);
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure I see the use cases or need for specifying Nested and Recursively 🤔
Nested is a part of how we define equivalency.
Recursively is an implementation detail of how we verify equivalency.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

During several of my masterclasses, the students mentioned that it was not obvious that this method would be called recursively. By adding this to the name, it would be enough of a hint to think about the consequences.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Playing the devil's advocate:
Why do you need to know it's recursive?
If I call a Sort function on sequence I don't know whether it's recursively or iteratively implemented, only that upon return I have a sorted sequence.

brain storming some names:

  • ValidateEquivalency
  • Validate (since the Equivalency part is already part of the interface name)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You need to make sure it's re-entrant. But I fail to come up with a good example of that right now..

@dennisdoomen
Copy link
Member Author

I meant with the planned improvements in the bullet list ;-)

@jnyrup
Copy link
Member

jnyrup commented May 1, 2021

A rework of IEquivalencyStep sounds great!

  • I've never fully understood (or maybe I just forgot) the Boolean that Handle returns.
  • Combining CanHandle and Handle to a single TryHandle could also give a small performance boost, as we currently throw away the knowledge we obtain in CanHandle.
  • If we reworking IEquivalencyStep anyways, I'm fine with renaming it.
    • Just note that renames in our extensibility types will make migration for our users a bit harder.

For the remaining I'm not sure what the consequences are, but I have no immediate objects.

@dennisdoomen dennisdoomen force-pushed the Refactor/EquivalencyValidator2021 branch 3 times, most recently from b110fbf to 47973ee Compare May 2, 2021 11:29
@dennisdoomen dennisdoomen force-pushed the Refactor/EquivalencyValidator2021 branch 13 times, most recently from 9e4ef3e to 93206dc Compare May 9, 2021 07:18
@dennisdoomen dennisdoomen force-pushed the Refactor/EquivalencyValidator2021 branch 9 times, most recently from 86509ca to 6c77344 Compare May 16, 2021 07:00
@dennisdoomen dennisdoomen marked this pull request as ready for review May 16, 2021 07:00
@dennisdoomen dennisdoomen requested a review from jnyrup May 16, 2021 07:00
@dennisdoomen dennisdoomen changed the title More cleaning of EquivalencyValidator Major refactoring of the EquivalencyValidator May 16, 2021
@dennisdoomen
Copy link
Member Author

dennisdoomen commented May 16, 2021

@jnyrup I tried to split the changes into atomic commits, but almost everything affected everything else, at some point I gave up. But if it would help you, I could split them up in commits that won't compile individually. Just let me know.

docs/_pages/upgradingtov6.md Outdated Show resolved Hide resolved
docs/_pages/releases.md Outdated Show resolved Hide resolved
Src/FluentAssertions/Equivalency/IValidateEquivalency.cs Outdated Show resolved Hide resolved
Src/FluentAssertions/Equivalency/IValidateEquivalency.cs Outdated Show resolved Hide resolved
@dennisdoomen dennisdoomen force-pushed the Refactor/EquivalencyValidator2021 branch from 6c77344 to aac544d Compare May 16, 2021 12:54
@dennisdoomen dennisdoomen requested a review from jnyrup May 16, 2021 12:54
@dennisdoomen dennisdoomen force-pushed the Refactor/EquivalencyValidator2021 branch from aac544d to bfbd555 Compare May 18, 2021 13:53
* Reduce usage of AssertionScope.Current in EquivalencyValidator
* Renamed `IEquivalencyValidator` to a more role-based named
* Encapsulated `isComplexTypeMap` into `ComplexTypeMap`
* Reduce usage of `AssertionScope.Current` in `EquivalencyValidator`
* Attach `CyclicReferenceDetector` to the `EquivalencyValidationContext` instead of the current `AssertionScope` and expose `IsCycleReference` through `IEquivalencyValidationContext`
* Removed `CanHandle` from `IEquivalencyStep`
* Changed the return value of `IEquivalencyStep.Handle` into an enum with values `ContinueWithNext` and `AssertionCompleted`
* Split-off the subject and expectation from `IEquivalencyValidationContext` into `Comparands` to emphasize what we're comparing
* Renamed `EquivalencyStepCollection` to `EquivalencyPlan` as well as the corresponding `AssertionOptions` property
* Added the typed base-class `EquivalencyStep<T>` for convenience
* Updated the extensibility documentation as well as the upgrade section
@dennisdoomen dennisdoomen force-pushed the Refactor/EquivalencyValidator2021 branch from bfbd555 to 7f1dbfa Compare May 18, 2021 13:55
@dennisdoomen dennisdoomen requested review from jnyrup and removed request for jnyrup May 18, 2021 13:59
@dennisdoomen dennisdoomen merged commit 684b578 into fluentassertions:develop May 18, 2021
@dennisdoomen dennisdoomen deleted the Refactor/EquivalencyValidator2021 branch May 18, 2021 18:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

IEquivalencyStep has an unclear return value
2 participants