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

Improve Using<TMember> #1494

Merged
merged 1 commit into from Feb 21, 2021
Merged

Improve Using<TMember> #1494

merged 1 commit into from Feb 21, 2021

Conversation

jnyrup
Copy link
Member

@jnyrup jnyrup commented Feb 13, 2021

Restrict WhenTypeIs<TMemberType> to types assignable to TMember.
This should catch NullReferenceException or InvalidCastException in CreateFromEquivalencyValidationContext at compile-time.
image

The snippet below might have worked, but it assumes that the subject and expectation are both non-null.

.Using<int>()
.WhenTypeIs<int?>()

Instead, you now have to make this assumption more explicit by using Nullable<int>.Value

Using<int?>(e => e.Subject.Value.Should().Be(e.Subject.Value))

Make WhenTypeIs<TMemberType> optional unless when chaining further.
For the most cases TMemberType is exactly TMember and in those cases WhereTypeIs<>() is noisy.
By adding an implicit cast operator we can avoid that.
The exception being when we need more chaining after Using<>, see the added When_overriding_with_custom_assertion_it_should_be_chainable.

Avoid trying to cast null to non-nullable type.
This handles when subject or expectation is null, but Using<int> would either:

  • cast null to int leading to a NullReferenceException`, or
  • use default(int) when Expectation is null, which would make null and 0 equal.

As far as I can read from #33 the changes in When_a_nullable_property_is_overriden_with_a_custom_assertion_it_should_use_it are fine, as the original bug report was about using<long?> didn't match a non-null long?

This fixes #799

@jnyrup
Copy link
Member Author

jnyrup commented Feb 13, 2021

Missing release notes and docs

Do you think this requires a section in the upgrading guide?

@dennisdoomen
Copy link
Member

Do you think this requires a section in the upgrading guide?

I think so. It took me a couple of times before I understood what this PR was trying to do.

@jnyrup
Copy link
Member Author

jnyrup commented Feb 14, 2021

Hmm... wondering if the implicit cast operator creates more confusion than value?
With this PR one can omit WhenTypeIs<T>() but only when Using<T>() is the last part of the chain.

@dennisdoomen
Copy link
Member

Hmm... wondering if the implicit cast operator creates more confusion than value?
With this PR one can omit WhenTypeIs<T>() but only when Using<T>() is the last part of the chain.

But it does simplify the use of this syntax.

@jnyrup
Copy link
Member Author

jnyrup commented Feb 15, 2021

Yes, it simplifies the usage, when having using at the last term.

To give a more concrete example of the potential confusion about when WhenTypeIs is required and when it's optional.

Say I start out with this snippet and want to exclude a property ExcludeMe

subject.Should().BeEquivalentTo(expected, opt => opt.
    .Using<int>(...)

Adding Excluding() directly after Using<int>() does not compile, as Excluding is not a member of Restriction and the implicit conversion has not kicked in yet.

subject.Should().BeEquivalentTo(expected, opt => opt.
    .Using<int>(...)
    .Excluding(e => e.ExcludeMe)

To make it work I have to either put Excluding before Using

subject.Should().BeEquivalentTo(expected, opt => opt.
    .Excluding(e => e.ExcludeMe)
    .Using<int>(...)

or add WhenTypeIs as one has to do right now.

subject.Should().BeEquivalentTo(expected, opt => opt.
    .Using<int>(...)
    .WhenTypeIs<int>()
    .Excluding(e => e.ExcludeMe)

@dennisdoomen
Copy link
Member

I see your point. In the current implementation, it's immediately clear what your options are since Intellisense knows what can be chained at any point in time.

@jnyrup
Copy link
Member Author

jnyrup commented Feb 21, 2021

I see your point. In the current implementation, it's immediately clear what your options are since Intellisense knows what can be chained at any point in time.

Was that comment towards keeping or leaving out the implicit conversion operator?

@dennisdoomen
Copy link
Member

Was that comment towards keeping or leaving out the implicit conversion operator?

Merely that I got your doubts about the whole thing. But I don't have a strong opinion on this.

Restrict `WhenTypeIs<TMemberType>` to types assignable from `TMember`.

Make `WhenTypeIs<TMemberType>` optional unless when chaining further.

Avoid trying to cast null to non-nullable type.
@jnyrup jnyrup merged commit 708b537 into fluentassertions:develop Feb 21, 2021
@jnyrup jnyrup deleted the Issue799 branch February 21, 2021 09:22
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.

Changing the behavior of WhenTypeIs for value types
2 participants