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

Prevent TypeCatalogueInstanceProvider from throwing a first chance MissingMethodException instantiating DefaultEnumerableValueFormatter #1791

Merged
merged 1 commit into from
Sep 1, 2020

Conversation

akrock
Copy link
Contributor

@akrock akrock commented Aug 31, 2020

  • Exclude FIE Assembly from TypeCatalogue
  • Adjust CanBeInstantiatedAs to include checking for a public no-arg constructor.

All the tests were passing on .NET Core, I had issues w/ getting the full framework to run on my machine due to missing very old framework versions.

@akrock
Copy link
Contributor Author

akrock commented Aug 31, 2020

Addresses #1790

@akrock
Copy link
Contributor Author

akrock commented Aug 31, 2020

Oops forgot all the niceties of .net land that have come over the years, will fix the Array.Empty error.

Copy link
Member

@thomaslevesque thomaslevesque left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @akrock! I have a few minor comments (some inline in the diff, and some here).

In the issue, @blairconrad said this:

we'd need to explicitly supply StringDummyFactory to the DynamicDummyFactory)

I think this should be changed in RootModule.cs. In fact, I'm a bit surprised that it doesn't break a test...

Also, you still have a build error. This is because in .NET Standard 1.6, TypeInfo doesn't inherit from Type, so you can't call HasDefaultConstructor on it. Changing typeInfo.HasDefaultConstructor() to type.HasDefaultConstructor() should fix it

src/FakeItEasy/TypeExtensions.cs Outdated Show resolved Hide resolved
src/FakeItEasy/TypeExtensions.cs Outdated Show resolved Hide resolved
@thomaslevesque
Copy link
Member

In fact, I'm a bit surprised that it doesn't break a test...

Ah, I see. StringDummyFactory was introduced as an optimization. It works without it, it's just inefficient.

@blairconrad
Copy link
Member

blairconrad commented Aug 31, 2020

Ah, I see. StringDummyFactory was introduced as an optimization. It works without it, it's just inefficient.

Oh. I thought without it we might also have a first chance exception. I could be wrong. Either way, it'd be nice to keep using StringDummyFactory as @thomaslevesque suggested.

@akrock
Copy link
Contributor Author

akrock commented Aug 31, 2020

@thomaslevesque @blairconrad

I explicitly added the StringDummyFactory in RootModule to the collection of IDummyFactory that is discovered through the TypeCatalogue... is this what you had in mind?

Copy link
Member

@blairconrad blairconrad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@akrock, nice work. I did make one minor inline comment (about is object), but aside from that, I am very happy. I'd even say you could squash the commits up as you make that last change, only I don't want things to shift too much under @thomaslevesque.

I explicitly added the StringDummyFactory in RootModule to the collection of IDummyFactory that is discovered through the TypeCatalogue... is this what you had in mind?

Thanks for that.
Truth be told, I hadn't thought much about where the addition would be. I am happy for it to be in RootModule. An alternative would be for it to be nested inside the DynamicDummyFactory and supplied by its own constructor, but your approach seems as good to me.

It may come up in future discussion, so I'll comment here.
I considered asking for a change in scanning-for-extension-points.md, removing "its own assembly", but then I thought we might as well leave it. FakeItEasy is just doing a different kind of search. A manual one. @thomaslevesque , if you disagree, I don't mind if the bullet point goes away. Honestly, I think the behaviour is transparent to clients either way.

src/FakeItEasy/TypeExtensions.cs Outdated Show resolved Hide resolved
blairconrad
blairconrad previously approved these changes Sep 1, 2020
Copy link
Member

@blairconrad blairconrad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, @akrock!

@thomaslevesque
Copy link
Member

I considered asking for a change in scanning-for-extension-points.md, removing "its own assembly", but then I thought we might as well leave it. FakeItEasy is just doing a different kind of search. A manual one. @thomaslevesque , if you disagree, I don't mind if the bullet point goes away. Honestly, I think the behaviour is transparent to clients either way.

I don't mind leaving it as is

thomaslevesque
thomaslevesque previously approved these changes Sep 1, 2020
Copy link
Member

@thomaslevesque thomaslevesque left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me, thanks @akrock!
Could you please squash the commits before we merge?

- Exclude FIE Assembly from TypeCatalogue
- Adjust CanBeInstantiatedAs to include checking for a public no-arg constructor.
@akrock
Copy link
Contributor Author

akrock commented Sep 1, 2020

Squashed.

Thanks to you two for the great work you do on this project @thomaslevesque & @blairconrad

@thomaslevesque thomaslevesque merged commit 1fcf74e into FakeItEasy:master Sep 1, 2020
@thomaslevesque
Copy link
Member

Thanks again, @akrock !

@blairconrad
Copy link
Member

Indeed, thanks for your work, @akrock!

@akrock akrock deleted the fix-1790 branch September 2, 2020 00:01
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.

None yet

3 participants