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

Add Have/NotHaveSameCount for Dictionaries #1178

Merged
merged 3 commits into from Nov 11, 2019

Conversation

@veleek
Copy link
Contributor

veleek commented Nov 6, 2019

Adding support for HaveSameCount and NotHaveSameCount for dictionaries. This was almost directly cribbed from the existing CollectionAssertions (including test cases).

Resolves #1176

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 affects the documentation, please include your changes to documentation.md in this pull request so the documentation will appear on the website.
@veleek veleek changed the title [WIP] Add Have/NotHaveSameCount for Dictionaries Add Have/NotHaveSameCount for Dictionaries Nov 6, 2019
{
Guard.ThrowIfArgumentIsNull(otherCollection, nameof(otherCollection), "Cannot compare dictionary count against a <null> collection.");

if (ReferenceEquals(Subject, null))

This comment has been minimized.

Copy link
@dennisdoomen

dennisdoomen Nov 7, 2019

Member

Considering using is null.

This comment has been minimized.

Copy link
@jnyrup

jnyrup Nov 7, 2019

Collaborator

The reason is null isn't used in CollectionAssertions.HaveSameCount is because TSubject is not constrained to reference types and is null can only be used on reference types.

This comment has been minimized.

Copy link
@dennisdoomen

dennisdoomen Nov 9, 2019

Member

SonarQube keeps telling me (in another project) to compare to default(T) ;-)

@veleek

This comment has been minimized.

Copy link
Contributor Author

veleek commented Nov 8, 2019

If there are no other concerns, what's the process for getting this merged?

@dennisdoomen dennisdoomen requested a review from jnyrup Nov 9, 2019
@jnyrup

This comment has been minimized.

Copy link
Collaborator

jnyrup commented Nov 9, 2019

@veleek Could I ask you to change the two ReferenceEquals(Subject, null) to Subject is null.
That will align with the rest of the project where using (generically constrained) reference types?
Then I'll be happy to push to the merge button 👍

Use `is null` instead of `ReferenceEquals(..., null)`.
@jnyrup
jnyrup approved these changes Nov 11, 2019
@jnyrup jnyrup merged commit c477f8b into fluentassertions:master Nov 11, 2019
1 check passed
1 check passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
@jnyrup

This comment has been minimized.

Copy link
Collaborator

jnyrup commented Nov 11, 2019

@veleek Thanks for the contribution!

@dennisdoomen

This comment has been minimized.

Copy link
Member

dennisdoomen commented Nov 12, 2019

Thanks for the contribution!

Indeed. Another nice little gem.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.