-
Notifications
You must be signed in to change notification settings - Fork 55
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
Adding Min,Max,Pow functionality to Lambda expressions #35
Conversation
Hi @victoriatolls, thanks for submitting this PR. Can you add a few unit tests for the new Min/Max/Pow functions? Best, |
test/NCalc.Tests/Lambdas.cs
Outdated
{ | ||
var expression = new Expression(input); | ||
var sut = expression.ToLambda<object>(); | ||
Assert.NotNull(sut()); |
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.
can you assert the correct return value here? e.g. for Min(3,2) it should equal 2
Hi all, This broke some functionality for us because we have our own implementations of As a solution, I propose we move them after the keywords and after the context lookup, something like the snippet at the end of this comment. My reasoning:
My proposal is something like: string functionName = function.Identifier.Name.ToLowerInvariant();
if (functionName == "if") {
_result = L.Expression.Condition(args[0], args[1], args[2]);
return;
} else if (functionName == "in") {
var items = L.Expression.NewArrayInit(args[0].Type, new ArraySegment<L.Expression>(args, 1, args.Length - 1));
var smi = typeof (Array).GetRuntimeMethod("IndexOf", new[] { typeof(Array), typeof(object) });
var r = L.Expression.Call(smi, L.Expression.Convert(items, typeof(Array)), L.Expression.Convert(args[0], typeof(object)));
_result = L.Expression.GreaterThanOrEqual(r, L.Expression.Constant(0));
return;
}
//"FindMethod" would be changed so it would just return null instead of throwing "method not found".
var mi = FindMethod(function.Identifier.Name, args);
if (mi != null) {
_result = L.Expression.Call(_context, mi.BaseMethodInfo, mi.PreparedArguments);
return;
}
// default function implementation like min, max, pow, and any other in the future.
switch (functionName) {
case "min":
var min_arg0 = L.Expression.Convert(args[0], typeof(double));
var min_arg1 = L.Expression.Convert(args[1], typeof(double));
_result = L.Expression.Condition(L.Expression.LessThan(min_arg0, min_arg1), min_arg0, min_arg1);
break;
case "max":
var max_arg0 = L.Expression.Convert(args[0], typeof(double));
var max_arg1 = L.Expression.Convert(args[1], typeof(double));
_result = L.Expression.Condition(L.Expression.GreaterThan(max_arg0, max_arg1), max_arg0, max_arg1);
break;
case "pow":
var pow_arg0 = L.Expression.Convert(args[0], typeof(double));
var pow_arg1 = L.Expression.Convert(args[1], typeof(double));
_result = L.Expression.Power(pow_arg0, pow_arg1);
break;
default:
throw new Exception($"method not found: {functionName}");
} @sklose I understand you're only passively maintaining the repo at the moment so I'm happy to prepare this, add tests and open a PR if you concur with the above. We've only just found out about this because we've only recently upgraded to the latest version. We had to rollback to a version just before this one though because of the breaking change. Thanks all, fadulalla |
Hi @fadulalla this sounds very reasonable to me. Happy to merge a PR with those changes. Best, |
No description provided.