Make ExpressionPromoter plugable #212
Conversation
| @@ -20,5 +20,6 @@ public interface IDynamicLinkCustomTypeProvider | |||
| /// <param name="typeName">The typename to resolve.</param> | |||
| /// <returns>A resolved <see cref="Type"/> or null when not found.</returns> | |||
| Type ResolveType([NotNull] string typeName); | |||
|
|
|||
There was a problem hiding this comment.
please remove this empty line.
| @@ -3,9 +3,9 @@ | |||
|
|
|||
| namespace System.Linq.Dynamic.Core.Parser | |||
| { | |||
| internal static class ExpressionPromoter | |||
| internal class ExpressionPromoter : IExpressionPromoter | |||
There was a problem hiding this comment.
| internal class ExpressionPromoter : IExpressionPromoter | |
| maybe if you have time ; can you create a simple test which tests this ExpressionPromoter? |
| { | ||
| public static Expression Promote(Expression expr, Type type, bool exact, bool convertExpr) | ||
| public Expression Promote(Expression expr, Type type, bool exact, bool convertExpr) |
There was a problem hiding this comment.
Add /// <inheritdoc cref="IExpressionPromoter.Promote(Expression, Type, bool, bool)"/> here.
| namespace System.Linq.Dynamic.Core.Parser | ||
| { | ||
| /// <summary> | ||
| /// Expression promoter is used whe matching |
There was a problem hiding this comment.
Typo and maybe add some more text to explain what this is doing.
| /// Promote an expression | ||
| /// </summary> | ||
| /// <param name="expr">Source expression.</param> | ||
| /// <param name="type">Destionation data type to promote to.</param> |
| /// <param name="parsingConfig"></param> | ||
| public MethodFinder(ParsingConfig parsingConfig) | ||
| { | ||
| this._parsingConfig = parsingConfig; |
| /// <summary> | ||
| /// Gets or sets the <see cref="IExpressionPromoter"/>. | ||
| /// </summary> | ||
| public IExpressionPromoter ExpressionPromoter |
There was a problem hiding this comment.
please add a unit test where you implement a different ExpressionPromoter or use Mock<IExpressionPromoter>
| @@ -20,8 +21,16 @@ public class ParsingConfig | |||
| EvaluateGroupByAtDatabase = true | |||
| }; | |||
|
|
|||
| /// <summary> | |||
| /// The custom type provider. | |||
There was a problem hiding this comment.
comment is not required for private
| private IDynamicLinkCustomTypeProvider _customTypeProvider; | ||
|
|
||
| /// <summary> | ||
| /// The expression promoter. |
There was a problem hiding this comment.
comment is not required for private
| { | ||
| public static bool ContainsMethod(Type type, string methodName, bool staticAccess, Expression[] args) | ||
| /// <summary> | ||
| /// The parsing config |
There was a problem hiding this comment.
comment is not required for private
Codecov Report
@@ Coverage Diff @@
## master #212 +/- ##
=========================================
+ Coverage 82.49% 82.69% +0.2%
=========================================
Files 37 37
Lines 3702 3716 +14
Branches 508 509 +1
=========================================
+ Hits 3054 3073 +19
Misses 501 501
+ Partials 147 142 -5
Continue to review full report at Codecov.
|
|
@david-garcia-garcia Can you take a look at my review comments? |
|
Took care of all comments + added some very basic test coverage. |
Now that the library allows to instantiate more .Net types easily, Expression promoter needs to be more powerful depending on the needs of the destination types.
This PR makes it plugable and replaceable from outside the library.