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 `NotBeApproximately` version accepting nullable subject and expected #939

Conversation

@krajek
Copy link
Contributor

krajek commented Oct 5, 2018

Followup of #934.
Just as previously, I start the PR with the float version. Upon approval, I will copy the logic for decimal? and `double?' overloads.

There are two interesting cases:

  • When the subject has value, but unexpectedValue does not. Because reversed case throws, for symmetry, I suggest throwing and this is how I implemented it.
  • When both subject and unexpectedValue are null. In that case, I decided not to throw, as semantically there is no way to determine the difference between them. However, I am not sure about this one, please review.
@dennisdoomen

This comment has been minimized.

Copy link
Member

dennisdoomen commented Oct 6, 2018

Hmm, we don't seem to have a BeApproximately that takes a nullable value as the expectation. So why would we want to have that for NotBeApproximately?

@krajek

This comment has been minimized.

Copy link
Contributor Author

krajek commented Oct 6, 2018

Hmm, we don't seem to have a BeApproximately that takes a nullable value as the expectation. So why would we want to have that for NotBeApproximately?

And what about merged PR #934? :-)

@dennisdoomen

This comment has been minimized.

Copy link
Member

dennisdoomen commented Oct 6, 2018

Sigh. I wasn't paying attention.

@krajek krajek force-pushed the krajek:not_be_approximately_subject_and_expected_negative branch from e855ab7 to 4895f8c Oct 7, 2018
@krajek krajek changed the title [WIP] Add `NotBeApproximately` version accepting nullable subject and expected Add `NotBeApproximately` version accepting nullable subject and expected Oct 7, 2018
@krajek

This comment has been minimized.

Copy link
Contributor Author

krajek commented Oct 7, 2018

I have just implemented double? and decimal? versions. Ready for merge.

@jnyrup

This comment has been minimized.

Copy link
Collaborator

jnyrup commented Oct 7, 2018

I think I disagree with the null cases.

To write up the three cases in prose form.

  • null should not be approximately null
  • null should not be approximately 1
  • 1 should not be approximately null

The proposed implementation has the output:

  • null should not be approximately null => NotThrow
  • null should not be approximately 1 => Throw
  • 1 should not be approximately null => Throw

I'm more inclined to say that the output should be

  • null should not be approximately null => Throw
  • null should not be approximately 1 => NotThrow
  • 1 should not be approximately null => NotThrow
@dennisdoomen

This comment has been minimized.

Copy link
Member

dennisdoomen commented Oct 8, 2018

I'm starting to think we should not be fixing this scenario at all. Why would you be able to compare to null in the first place

@jnyrup

This comment has been minimized.

Copy link
Collaborator

jnyrup commented Oct 8, 2018

@dennisdoomen By "in general", do you then mean both NotBeApproximately and BeApproximately for nullables, or only NotBeApproximately?

The use case for BeApproximately for nullable expectations was when doing equivalency comparison.

e.g.

var actual = new { B = (double?)1 };

var expected = new { B = (double?)2 };

actual.Should().BeEquivalentTo(expected,
    opt => opt.Using<double?>(ctx => ctx.Subject.Should().BeApproximately(ctx.Expectation, 1))
    .WhenTypeIs<double?>());
@dennisdoomen

This comment has been minimized.

Copy link
Member

dennisdoomen commented Oct 8, 2018

Alright. In that case, I agree with the last proposal @jnyrup did.

@krajek

This comment has been minimized.

Copy link
Contributor Author

krajek commented Oct 8, 2018

First of all, if that is not too inconvenient, use "throws/does not throw" terminology instead of "true/false". Somehow, every time I read it, I am confused and have to translate between terms ;-).

I recognize your point of view. Let's break it up in two sub-topics for even more clarity.

  1. Subject or expected has a value. Note, that case when the subject is null was present before the PR and it threw. As far as I understand, you would like to change the behaviour and not throw in that case. Are you aware this is a breaking change? I think that case when expected is null and the subject has value should be symmetrical, no matter the decision. @jnyrup please comment.

  2. Both the subject and expected have value. I agree that my implementation should be reversed and in case of null on both sides, I will throw.

@jnyrup

This comment has been minimized.

Copy link
Collaborator

jnyrup commented Oct 8, 2018

Now I see what you refer to by symmetrical, that is symmetrical to the NotBeApproximately for nullable subject but non-nullable expectation. I thought you somehow meant mirrored behavior of BeApproximately for nullable subject and expectation.

To recap:
The existing tests
When_asserting_not_approximately_and_nullable_double_has_no_value_it_should_throw and
When_nullable_double_has_no_value_it_should_throw()

both throws when subject is null.

I'm not sure how we want to handle this.
I both think that :

  • When_asserting_not_approximately_and_nullable_double_has_no_value_it_should_throw() is wrong, and that
  • The outcome of NotBeApproximately for nullable subject should be the same whether you have a nullable with a value or a non-nullable value.
@jnyrup

This comment has been minimized.

Copy link
Collaborator

jnyrup commented Oct 10, 2018

So there are three possible directions to take:

  1. Align NotBeApproximately(double?) to NotBeApproximately(double) -- that
    is throw when subject is null regardless of the expectation,
  2. Let NotBeApproximately(double?) have different behavior when subject is null than for NotBeApproximately(double), or
  3. Fix NotBeApproximately(double) -- it is currently acting as HaveValue().And.NotBeApproximately() (breaking change)

Any opinions @dennisdoomen

@dennisdoomen

This comment has been minimized.

Copy link
Member

dennisdoomen commented Oct 11, 2018

I think that @jnyrup 's original proposal still holds, both for NotBeApproximately(double) and NotBeApproximately(double?). But I agree it's every option discussed so far has merits.

  • null should not be approximately null => Throw
  • null should not be approximately 1 => NotThrow
  • 1 should not be approximately null => NotThrow
@krajek

This comment has been minimized.

Copy link
Contributor Author

krajek commented Oct 11, 2018

Likewise, I think that each approach has its pros and cons but I like the one you guys described the most.
@dennisdoomen Just to double-check, before I start fixes, please confirm that you accept the fact that case

  • null should not be approximately 1 => NotThrow

is a breaking change.

@dennisdoomen

This comment has been minimized.

Copy link
Member

dennisdoomen commented Oct 11, 2018

I accept ;-)

@jnyrup

This comment has been minimized.

Copy link
Collaborator

jnyrup commented Oct 11, 2018

I tracked down the PR, where the author apparently carelessly copy/pasted BeApproximately into NotBeApproximately without flipping the null check for subject.
It's really embarrassing when that person is yourself 😞

@dennisdoomen

This comment has been minimized.

Copy link
Member

dennisdoomen commented Oct 11, 2018

Or you have been investing enough in this project that you forgot how much ;-)

@eNeRGy164

This comment has been minimized.

Copy link
Contributor

eNeRGy164 commented Oct 12, 2018

@jnyrup I would blame the person accepting and merging the PR. 😉

@krajek krajek force-pushed the krajek:not_be_approximately_subject_and_expected_negative branch from 4895f8c to a802df3 Oct 13, 2018
@krajek

This comment has been minimized.

Copy link
Contributor Author

krajek commented Oct 13, 2018

Behaviour adjusted, ready to merge.

@jnyrup
jnyrup approved these changes Oct 18, 2018
Copy link
Collaborator

jnyrup left a comment

@krajek sorry for the late review, I must have missed out on a notification on your latest commit.

@jnyrup jnyrup requested a review from dennisdoomen Oct 18, 2018
@jnyrup jnyrup merged commit b74b041 into fluentassertions:master Oct 19, 2018
1 check passed
1 check passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
@krajek krajek deleted the krajek:not_be_approximately_subject_and_expected_negative branch Nov 12, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.