-
-
Notifications
You must be signed in to change notification settings - Fork 794
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
Drop System.ValueTuple
NuGet dependency
#721
Conversation
People are still experiencing assembly versioning problems on the .NET Framework because of the NuGet packages `System.Threading.Tasks.Exten- sions` and `System.ValueTuple` referenced by Moq. It is fairly trivial to get rid of the latter dependency... so let's get rid of it! (It means we can no longer use tuple syntax everywhere in Moq's code base, but as we haven't made excessive use of it so far, that's perhaps a small price to pay.)
["System.ValueTuple`5"] = CreateValueTupleOf, | ||
["System.ValueTuple`6"] = CreateValueTupleOf, | ||
["System.ValueTuple`7"] = CreateValueTupleOf, | ||
["System.ValueTuple`8"] = CreateValueTupleOf, |
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.
What this means is that Moq now recognises System.ValueTuple<…>
types by name instead of by strict type identity. This mimicks what the C# compiler already does.
(Unlike the C# compiler, Moq however won't validate the structural makeup of the matched types, so Moq's default value providers will not work correctly with such user types that don't have the expected constructor signature.)
return this.factories.TryGetValue(handlerKey, out Func<Type, Mock, object> factory) ? factory.Invoke(type, mock) | ||
Func<Type, Mock, object> factory; | ||
return this.factories.TryGetValue(handlerKey , out factory) ? factory.Invoke(type, mock) | ||
: this.factories.TryGetValue(handlerKey.FullName, out factory) ? factory.Invoke(type, mock) | ||
: this.GetFallbackDefaultValue(type, mock); |
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.
The default value providers will now do two dictionary lookups instead of one in many cases. In theory, that's a slight performance degradation, but it would be surprising if this mattered much in practice. (A dictionary lookup is likely nothing when compared to all the reflection and code emitting going on elsewhere.)
@@ -169,4 +169,25 @@ internal static (Expression Object, MethodInfo Method, IReadOnlyList<Expression> | |||
expression.ToStringFixed())); | |||
} | |||
} | |||
|
|||
internal readonly struct CallInfo |
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.
We could declare a few generic tuple types instead of this and other concrete readonly struct
s. For now, that degree of reusability isn't needed, and we spare the JITter some additional work by avoiding generics. (And, perhaps more importantly, declaring concrete, non-generic types gives us a means of giving the constituent parts names more meaningful than just Item1
, Item2
, etc.)
this.arguments = arguments; | ||
} | ||
|
||
public void Deconstruct(out Expression expression, out MethodInfo method, out IReadOnlyList<Expression> arguments) |
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.
Using a Deconstruct
instead of just making the fields public
means that we can still use tuple syntax / deconstruction in assignments.
People are still experiencing assembly versioning problems on the .NET Framework because of the NuGet packages
System.Threading.Tasks.Extensions
andSystem.ValueTuple
referenced by Moq. (See e.g. the recent #719.)It is fairly trivial to get rid of the latter dependency... so let's get rid of it! (It means we can no longer use tuple syntax everywhere in Moq's code base, but as we haven't made excessive use of it so far, that's perhaps a small price to pay.)