Parse and Compile separation #207

[ealeykin]

A proposed refactored sample version of DynamicExpresso in terms of Parse and Compile separation.

#207

[ealeykin]

I had to comment out some tests related to Invoke with named parameters, like omitting the unused parameter or passing out of order parameters etc.

Also haven't found out the cause of failing tests related to LambdaExpression option. target.Parse("list.Select(str => str.Length)") - str here if of type Object.

[davideicardi]

Thank you @halamah . I will study your changes and investigate about failing tests.

Can you sycnronize your branch with master (rebase) so that we always compare it with the latest version?

cc @metoule Any feedback on this?

[ealeykin]

@davideicardi @metoule - I've changed my PR as the initial one was really too cumbersome. Here I've just copy/pasted the interpreter to a new ExpressionInterpreter class with relevant changes. All these is fully backward compatible since the Interpreter class wasn't changed.

So having this new ExpressionInterpter + InterpreterExtensions I can do basically the same, but the client has more obvious control over produced Expression.

Plus I've just added a simple extension to Eval an expression without even touching an Interpreter class.

var interpreter = new ExpressionInterpreter();
var result = interpreter.Eval("x + 1", x => 2);
Assert.Equals(result, 3);

[metoule]

If ExpressionInterpreter is mostly a copy / paste of Interpreter, wouldn't it be possible to keep the Interpreter class, while moving all the Parse / Eval methods to extension methods? There could be two extension classes: one with the existing methods, and another one with your new methods.

If that's not possible, maybe at least create an abstract class that will be the base of both ExpressionInterpreter and Interpreter?

[ealeykin]

I've tried both suggestions and

1. There will be name collisions for Parse methods (new one returns different type) and I don't really like to rename it. Also it will be unclear which extension to use to get expected return value (Lambda or ParseResult). We could probably name those differently, but the usage more convoluted then.

2. Base class actually turns into a wrapper over Settings and Visitors - and this will work, but the intention is to mark current Interpreter and Lambda classes as Obsolete and don't support those further. And having this base class forces to support both Interpreters (makes more sense to me to keep the old one as is). All new changes will come to new ExpressionInterpreter and Extension methods only.

[metoule]

OK, I see your point. If I understand correctly, the main issue is the Lambda class. In that case, can't the Lambda class becomes your current ParserResult class, with extension methods to cover the old interface? Would that work?

[ealeykin]

Yes, it could. But I'm note quite sure this will be "pretty" or even feasible. Current Lambda implementation stores compiled result and calling Invoke uses that already compiled lambda. Also there is a trick with UsedParameters - the issue this PR comes from (just to remind if a SetExpression text contains input parameter and a parent one doesn't - it won't be propagated).

On the other hand ParseResult is a raw parsed Expression (which is the most valuable part of the library) that could be used in numerous ways without compilation. Later if one needs to invoke the expression he must to compile it first.

Maybe there is some way to do what you suggest, but nothing comes to my mind so far without breaking backward compatibility.

[davideicardi]

On the other hand ParseResult is a raw parsed Expression (which is the most valuable part of the library) that could be used in numerous ways without compilation. Later if one needs to invoke the expression he must to compile it first.

@halamah I completely agree on that sentence. It would be nice to split up the parse and compile phase.
I'm not sure how to do it in the best way but we this is a good start.

[metoule]

Yes, ParseResult is clearly a step in the right direction! I believe the Lambda class can be made to encapsulate a ParseResult, while retaining the old methods. That way, it will be possible to add new methods that return a ParseResult, while still keeping the old way of doing things to avoid breaking changes.

I checked your PR locally to see if I can find a way to do that :)

[metoule]

@halamah I couldn't find a way to update this PR, so I created a new one based on this: #216