Remove redundant code from matcher infrastructure #514
Conversation
stakx
force-pushed
the
matchers
branch
2 times, most recently
from
bd65ab0
to
b305ccc
Compare
| @@ -202,16 +202,11 @@ public static Expression PartialMatcherAwareEval(this Expression expression) | |||
|
|
|||
| private static bool ReturnsMatch(MethodCallExpression expression) | |||
| { | |||
| if (!expression.Method.IsDefined(typeof(AdvancedMatcherAttribute), true)) | |||
There was a problem hiding this comment.
One would expect there to be a check for the obsolete [Matcher] attribute, but such a check wasn't there previously. It might be good to investigate more thoroughly exactly when ReturnsMatch gets used.
| @@ -61,11 +66,6 @@ public static IMatcher CreateMatcher(Expression expression, bool isParams) | |||
| // the values are ints, but if the method to call | |||
| // expects, say, a double, a Convert node will be on | |||
| // the expression. | |||
| if (isParams && (expression.NodeType == ExpressionType.NewArrayInit || !expression.Type.IsArray)) | |||
There was a problem hiding this comment.
This gets moved up a few lines because the preceding comment belongs with code further below; but shouldn't the type conversion code be the first thing in the method instead?
| var matcher = new MatcherAttributeMatcher(); | ||
| matcher.Initialize(originalExpression); | ||
| return matcher; | ||
| return new MatcherAttributeMatcher(call); |
There was a problem hiding this comment.
Passing call instead of originalExpression is actually a small change, as the latter has had any type conversion node removed and the former hasn't. But it would seem more correct since code inside MatcherAttributeMatcher silently assumes that it's given a MethodCallExpression (and not a conversion node).
|
|
||
| namespace Moq.Matchers | ||
| { | ||
| internal class PredicateMatcher : IMatcher |
There was a problem hiding this comment.
This was used together with [AdvancedMatcher] on ItExpr.Is. Surprisingly, it doesn't appear to make any difference whatsoever whether that attribute is there or not. Perhaps check again whether diagnostic output (rendered expression in error messages) is affected, as that might be something not covered by any test.
| @@ -157,6 +157,8 @@ namespace Moq | |||
| /// </code> | |||
| /// </example> | |||
| [AttributeUsage(AttributeTargets.Method, Inherited = true)] | |||
| [EditorBrowsable(EditorBrowsableState.Never)] | |||
| [Obsolete("This feature has been deprecated in favor of `Match.Create`.")] | |||
There was a problem hiding this comment.
An existing remark in the documentation comment already states that this type has been deprecated, this just formalises it.
This extensibility point has never been part of the public API, and its internal use (in Moq's codebase) is minimal (`ItExpr.IsAny`). Furthermore, it does not appear to make any difference whatsoever. Therefore, let's remove it.
Now that `[AdvancedMatcher]` is gone, two-step initialization is no longer needed and we can get rid of this anti-pattern.
stakx
changed the title
|
|
||
| return false; | ||
| } | ||
| return Match.Create<Order>(o => o.Amount >= 1000); |
There was a problem hiding this comment.
This is what was originally used before the introduction of [AdvancedMatcher].
Source/MatcherFactory.cs
Outdated
|
|
||
| if (attr != null) | ||
| { | ||
| var matcher = attr.CreateMatcher(); | ||
| matcher.Initialize(originalExpression); | ||
| return matcher; | ||
| } | ||
| else if (staticMatcherMethodAttr != null) | ||
| #pragma warning disable 618 | ||
| else if (call.Method.IsDefined(typeof(MatcherAttribute), true)) |
There was a problem hiding this comment.
Minor optimization here: We don't need the actual attribute (GetCustomAttribute; above), we just need to know whether it is there or not (IsDefined). This call is about twice as fast.
The former is completely unnecessary if we just let the latter imple- ment `IMatcher` directly. (Ideally, it would be possible to rename `Match` to `Matcher` for consistency, but that's not easily possible as `Match` is part of the public API.)
The matcher infrastructure is currently somewhat difficult to understand:
Current situation:
MatchandMatch<T>, which is basically a wrapper around aPredicate<T>. (Unfortunately, this delegate type was chosen overFunc<T, bool>, but alas.)IMatcher, having several implementations in theMoq.Matchersnamespace.Matcher, which wraps aMatch<T>(which wraps aPredicate<T>).Regarding extensibility, the situation is similarly complex:
[Matcher]custom attribute, which appears to be the extension point for the infrastructure that was built first.Match.Createmethod, which supposedly deprecates[Matcher].[AdvancedMatcher]attribute, which is perhaps the newest addition and related toIMatcher, but hardly used at all.Changes:
This PR removes some of these things that aren't actually used, or don't make any difference whatsoever.
Matcheris removed, asMatchcan already do the same things.[Matcher]is marked as obsolete.[AdvancedMatcher]is removed, as it has no observable effect.