Skip to content

Conversation

@whymatter
Copy link
Contributor

Closes #1771

IMPORTANT

  • The code complies with the Coding Guidelines for C#.
  • The changes are covered by a new or existing set of unit tests which follow the Arrange-Act-Assert syntax such as is used in this example.
  • If the contribution adds a feature or fixes a bug, please update the release notes, which are published on the website.
  • If the contribution changes the public API the changes needs to be included by running AcceptApiChanges.ps1/AcceptApiChanges.sh.
  • If the contribution affects the documentation, please include your changes in this pull request so the documentation will appear on the website.

@whymatter whymatter changed the title Issue 1771 Add ThenExcluding to allow exclusion of members inside a collection Jan 20, 2022
@whymatter
Copy link
Contributor Author

@dennisdoomen I still have to do the release notes, however, you can already have a look at the ThenExcluding implementation. Unit tests are all green.

Copy link
Member

@dennisdoomen dennisdoomen left a comment

Choose a reason for hiding this comment

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

I recommend waiting with the rebase until #1789 is merged because it requires moving the tests to a new file.


public void ExtendPath(MemberPath nextPath)
{
memberToExclude = memberToExclude.Extend(nextPath, "[*]");
Copy link
Member

Choose a reason for hiding this comment

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

🤔 To align with the new WithMapping options, I think we should use [] for that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Please see this comment #1782 (comment)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done 🙂

@whymatter
Copy link
Contributor Author

Hi, sorry I was offline for a few days, looking forward to fix the issues next week :)

@whymatter whymatter marked this pull request as ready for review February 3, 2022 22:37
@whymatter
Copy link
Contributor Author

Updated the documentation and release notes, ran AcceptApiChanges.ps1 :)

@dennisdoomen dennisdoomen requested a review from jnyrup February 5, 2022 08:24
@dennisdoomen dennisdoomen changed the base branch from master to develop February 10, 2022 16:33
Made MemberPath and ExcludeMemberByPathSelectionRule extendible, added * as index qualifier, added MemberPathSegmentEqualityComparer to match MemberPath segments and respect * as index qualifier
* Added a function SetSelectedPath
* Rename EquivalencyAssertionOptionsBuilder to NestedExclusionOptionBuilder
* Renamed Extend* methods to Combine*
* Nested ThenExcluding test cases in a nested class
* Added Act/Assert comments
* Renamed tests
* Removed not important property Text from test data
* Drop Action and NotThrow
@jnyrup jnyrup added the feature label Feb 19, 2022
Copy link
Member

@jnyrup jnyrup left a comment

Choose a reason for hiding this comment

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

I tried to play a bit with this, but encountered something that seems a bit non-intuitive to me.

This passes as Collection is excluded from the equivalence

var subject = new { Member = "1", Collection = new[] { new { First = "A", Second = "B" } } };
var expected = new { Member = "1", Collection = new[] { new { First = "C", Second = "D" } } };

subject.Should().BeEquivalentTo(expected, opt => opt
    .Excluding(e => e.Collection));

Adding .ThenExcluding(e => e.Second) makes it fail as now only Collection.Second is excluded.

var subject = new { Member = "1", Collection = new[] { new { First = "A", Second = "B" } } };
var expected = new { Member = "1", Collection = new[] { new { First = "C", Second = "D" } } };

subject.Should().BeEquivalentTo(expected, opt => opt
    .Excluding(e => e.Collection).ThenExcluding(e => e.Second));

So the equivalency engine in fact includes Collection except its property Second.

I don't think the analogy of Include and ThenInclude from EF is completely translatable and invertible to Exclude and ThenExclude.
With ThenInclude you include something extra in addition to what's being included by Include.
If you have excluded something with Exclude, there's nothing left for ThenExclude to exclude.

I think e.g. combining Including with ThenExcluding comes closer to what's happening.

subject.Should().BeEquivalentTo(expected, opt => opt
    .Including(e => e.Collection).ThenExcluding(e => e.Second));

@whymatter
Copy link
Contributor Author

whymatter commented Feb 21, 2022

I see that Including followed by ThenExcluding is also an interesting case and I think we need it as well.
However, my intention was to help people using Excluding (like this guy #500).
If one has existing assertions that use Excluding and want to add attributes in nested collections and exclude them, it would be a pain to rewrite all test to use Including.

whymatter and others added 4 commits February 21, 2022 23:11
Co-authored-by: Jonas Nyrup <jnyrup@users.noreply.github.com>
Co-authored-by: Jonas Nyrup <jnyrup@users.noreply.github.com>
Co-authored-by: Jonas Nyrup <jnyrup@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Proposal to exclude properties on a nested list

5 participants