-
Notifications
You must be signed in to change notification settings - Fork 4.9k
Add FormatterServices.GetUninitializedObject, partial for #8133 #8570
Conversation
Hi @danmosemsft, I'm your friendly neighborhood .NET Foundation Pull Request Bot (You can call me DNFBOT). Thanks for your contribution! TTYL, DNFBOT; |
@weshaggard PTAL |
@danmosemsft, Thanks for signing the contribution license agreement so quickly! Actual humans will now validate the agreement and then evaluate the PR. |
{ | ||
public static class FormatterServicesTests | ||
{ | ||
private static Func<Type, Object> geo = FormatterServices.GetUninitializedObject; |
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.
Nit: geo
=> s_geo
It can also be readonly.
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.
That said... why use a delegate at all? Why not just call FormatterServices.GetUnitializedObject in each test?
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.
Readability :) I'll remove it.
LGTM |
@stephentoub What's the protocol now -- I wait for the tests to go green, then hit merge myself? |
Yes, though it'd be good for @weshaggard to take a look as well, in particular to make sure that the contract update is good. |
} | ||
|
||
[Theory] | ||
[MemberData(nameof(TestData))] |
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.
Nit: Does this need to be MemberData?
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.
Yes, MemberData seems to mean "invoke this static function as an interator to generate test data" while InlineData means "here lies inline data"
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.
Sorry, I mean the TestData() could be InlineData: there's nothing in the enumerator that needs to have its own method
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.
oh - you're quite right, it previously had but I modified it.
LGTM |
Add FormatterServices.GetUninitializedObject, partial for dotnet/corefx#8133 Commit migrated from dotnet/corefx@112ff9a
No description provided.