Consider types assignable to open generics (#954) by mdonoughe · Pull Request #955 · fluentassertions/fluentassertions

mdonoughe · 2018-10-31T02:38:29Z

I think I did this right, at least for BeAssignableTo. Base types and interfaces are searched for matching generic types.

Should BeDerivedFrom do the same thing but disallowing interfaces?

#954

IMPORTANT

The Pull Request is targeted at the master branch.
The code complies with the Coding Guidelines for C# 3.0, 4.0 and 5.0.
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.

dennisdoomen

Only minor comments and suggestions. Feel free to ignore them or apply then. I use these emojis.

Src/FluentAssertions/Types/TypeAssertions.cs

dennisdoomen · 2018-10-31T14:18:43Z

Src/FluentAssertions/Types/TypeAssertions.cs

+                if (subjectInfo.IsInterface && subjectInfo.IsGenericType &&
+                    subjectInfo.GetGenericTypeDefinition().IsSameOrEqualTo(definition))
+                {
+                    return true;


🙃 I personally find all these intermediate return statements a more difficult to read.

Is this better now?

Src/FluentAssertions/Types/TypeAssertions.cs

dennisdoomen · 2018-10-31T14:20:53Z

Tests/Shared.Specs/TypeAssertionSpecs.cs

        }

+        [Fact]
+        public void When_constructed_of_open_generic_it_succeeds()


🙃 We normally use the When_...._it_should_.... format for all our tests.
🤔 I don't get the when constructed. We're not constructing anything here, right?

Microsoft uses the term "constructed" when referring to a generic type that has its arguments filled in: https://docs.microsoft.com/en-us/dotnet/api/System.Type.IsConstructedGenericType?view=netframework-4.7.2 . I hadn't seen it before either.

I guess we're missing some kind of context for a group of specs so that we don't have to repeat the cause and effect.

dennisdoomen · 2018-10-31T14:22:31Z

Tests/Shared.Specs/TypeAssertionSpecs.cs

+        }
+
+        [Fact]
+        public void When_implementation_of_open_generic_interface_it_succeeds()


🙃 It feels like the name is missing a verb or something.

I was trying to emulate the "When_its_own_type_instance_it_succeeds" pattern the other BeAssignableTo methods were following without making the names super long.

mdonoughe · 2018-10-31T23:35:06Z

I've added in support for BeDerivedFrom too.

jnyrup

Thanks for taking up this issue 👍
If you have appetite for more, we need to get the following assertions consistent with respect to open generics.
Such that e.g. obj.Should().BeAssignableTo(typeof(IInterface<>)) and obj.GetType().Should().BeAssignableTo(typeof(IInterface<>)) have the same behavior.

TypeAssertions

[Not]Implement
[Not]BeDerivedFrom
[Not]BeAssignableTo

ReferenceTypeAssertions

[Not]BeOfType
[Not]BeAssignableTo

Src/FluentAssertions/Types/TypeAssertions.cs

mdonoughe · 2018-11-02T03:08:26Z

BeOfType already had open generics.

Is there a convention for both BeAssignableTos to use the same logic?

jnyrup · 2018-11-02T16:07:24Z

Yes, but NotBeOfType does not.

I'm not sure I understand your question.
If you're asking where to put shared logic, I would put it in TypeExtensions.cs.

jnyrup · 2018-11-05T22:22:22Z

Don't know if this is WIP, but BeAssignableTo<T> should probably call BeAssignableTo(Type) instead?

mdonoughe · 2018-11-06T01:34:42Z

It probably should, but it doesn't seem to make any difference since you can't pass open generics to methods like that. I assumed it did already.

mdonoughe · 2018-11-06T01:38:45Z

Actually should it? BeAssignableTo<T> casts the result to T, so for cases where it wouldn't match BeAssignableTo (shouldn't be any?) it would fail to cast even if it relied on BeAssignableTo to do the assertion.

dennisdoomen · 2018-11-07T17:00:21Z

@jnyrup can this be merged?

jnyrup · 2018-11-07T17:34:37Z

It looks very good now and feature wise could be merge.
But I would like to have both a positive and a negative test for BeAssignableTo, NotBeAssignableTo and NotBeOfType from ReferenceTypeAssertions when the expectation is an open generic.

dennisdoomen · 2018-11-07T19:08:48Z

Maybe it's better to request changes then. It makes the state of the PR more visual.

jnyrup

I would like to have both a positive and a negative test for BeAssignableTo, NotBeAssignableTo and NotBeOfType from ReferenceTypeAssertions when the expectation is an open generic.

jnyrup

@mdonoughe You can ignore the single failing test.
Testing timing related test is sometimes unstable, and all except one target framework that test passed.
I'm very satisfied now, so if you're good to go, I'll hit the merge button

Consider types assignable to open generics (fluentassertions#954)

d3da129

dennisdoomen approved these changes Oct 31, 2018

View reviewed changes

mdonoughe added 2 commits October 31, 2018 19:31

code clean up

10e7b3d

also consider open generics as base types

4672c52

jnyrup approved these changes Nov 1, 2018

View reviewed changes

Src/FluentAssertions/Types/TypeAssertions.cs Outdated Show resolved Hide resolved

Src/FluentAssertions/Types/TypeAssertions.cs Outdated Show resolved Hide resolved

Src/FluentAssertions/Types/TypeAssertions.cs Outdated Show resolved Hide resolved

code clean up

d5da030

mdonoughe added 2 commits November 3, 2018 12:46

make BeOfType NotBeOfType consistent

8ced798

make BeAssignableTo same for objects

93a5c70

jnyrup requested changes Nov 7, 2018

View reviewed changes

add more tests

28c277e

jnyrup approved these changes Nov 8, 2018

View reviewed changes

dennisdoomen merged commit a9abfa8 into fluentassertions:master Nov 8, 2018

jnyrup mentioned this pull request Nov 9, 2018

Type.Should().Be(Type) doesn't support open generics #954

Closed

mdonoughe deleted the open-generics branch November 11, 2018 03:29

Conversation

mdonoughe commented Oct 31, 2018 • edited by dennisdoomen Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

IMPORTANT

Uh oh!

dennisdoomen left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

dennisdoomen Oct 31, 2018

Choose a reason for hiding this comment

Uh oh!

mdonoughe Oct 31, 2018

Choose a reason for hiding this comment

Uh oh!

Uh oh!

dennisdoomen Oct 31, 2018

Choose a reason for hiding this comment

Uh oh!

mdonoughe Oct 31, 2018

Choose a reason for hiding this comment

Uh oh!

dennisdoomen Nov 1, 2018

Choose a reason for hiding this comment

Uh oh!

dennisdoomen Oct 31, 2018

Choose a reason for hiding this comment

Uh oh!

mdonoughe Oct 31, 2018

Choose a reason for hiding this comment

Uh oh!

mdonoughe commented Oct 31, 2018

Uh oh!

jnyrup left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mdonoughe commented Nov 2, 2018

Uh oh!

jnyrup commented Nov 2, 2018

Uh oh!

jnyrup commented Nov 5, 2018

Uh oh!

mdonoughe commented Nov 6, 2018

Uh oh!

mdonoughe commented Nov 6, 2018

Uh oh!

dennisdoomen commented Nov 7, 2018

Uh oh!

jnyrup commented Nov 7, 2018

Uh oh!

dennisdoomen commented Nov 7, 2018

Uh oh!

jnyrup left a comment

Choose a reason for hiding this comment

Uh oh!

jnyrup left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

mdonoughe commented Oct 31, 2018 •

edited by dennisdoomen

Loading