-
Notifications
You must be signed in to change notification settings - Fork 731
Add BeNaN and NotBeNaN assertions #2606
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 BeNaN and NotBeNaN assertions #2606
Conversation
Qodana for .NETIt seems all right 👌 No new problems were found according to the checks applied 💡 Qodana analysis was run in the pull request mode: only the changed files were checked Contact Qodana teamContact us at qodana-support@jetbrains.com
|
Pull Request Test Coverage Report for Build 8344341172Details
💛 - Coveralls |
| // Act | ||
| Action act = () => actual.Should().BeNaN(); | ||
|
|
||
| // Assert | ||
| act.Should().NotThrow(); |
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.
| // Act | |
| Action act = () => actual.Should().BeNaN(); | |
| // Assert | |
| act.Should().NotThrow(); | |
| // Act / Assert | |
| actual.Should().BeNaN(); |
We now combine assertions that are not supposed to throw into one.
(And following)
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.
Looks good 👍
I only have a few minor comments
Tests/FluentAssertions.Specs/Numeric/NumericAssertionSpecs.BeNaN.cs
Outdated
Show resolved
Hide resolved
| /// <summary> | ||
| /// Asserts that the float value is NaN. | ||
| /// </summary> | ||
| /// <param name="parent">The <see cref="NumericAssertions{T}"/> object that is being extended.</param> |
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.
🌱 I generally don't document these since they are not relevant for users.
| #region BeNaN | ||
|
|
||
| /// <summary> | ||
| /// Asserts that the float value is NaN. |
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.
🤔 I prefer to keep this more functional, e.g. like Asserts that a number is seen as not-a-number.
| return new AndConstraint<NullableNumericAssertions<double>>(parent); | ||
| } | ||
|
|
||
| private static void FailIfValueIsNaN(bool valueIsNaN, |
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.
🤔 I'm not a big fan of applying DRY like that. In most cases, duplicating the code makes the implementation much more readable.
4bfd607 to
8da273a
Compare
dennisdoomen
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.
It would be nice to update the numerictypes.md with the two new methods.
|
I did add them to the |
The XML summary on the |
I meant the |
Add the
BeNaNandNotBeNaNextension methods toNumericAssertionsExtensions.This fixes #2579.
IMPORTANT
./build.sh --target spellcheckor.\build.ps1 --target spellcheckbefore pushing and check the good outcome