nreco / lambdaparser

Runtime parser for string expressions (formulas, method calls). Builds dynamic LINQ expression tree and compiles it to lambda delegate.
http://www.nrecosite.com/
MIT License
309 stars 55 forks source link

Adding DateTime and Timespan support #17

Closed some1one closed 5 years ago

some1one commented 5 years ago

…tion, and a dictionary of value getters can be used

VitaliyMF commented 5 years ago

In this pull request I see 2 different things.

1) They are should be separated as different PRs 2) regarding changes in LambdaParser.cs, I've provided my thoughts on this in #4. Variables caching is rather specific thing, and this is just several lines of code. It is a little sense to hardcode it into library. 3) I'm ok to accept changes in 'LambdaParameterWrapper.cs' related to TimeSpan / DateTime handling, however code should be improved:

Code like this has too strong assumption:

if (c1.Value is TimeSpan || c2.Value is TimeSpan)
{
    var c1TimeSpan = c1.Value as TimeSpan?;
    var c2TimeSpan = c2.Value as TimeSpan?;
    return new LambdaParameterWrapper(c1TimeSpan + c2TimeSpan, c1.Cmp);
}

What if c1.Value is DateTime, and c2.Value is TimeSpan? This is also correct set of arguments for data-type + or -, but your code will not work correctly.

My suggestion is to use strong type checks for + and - operations, smth like:

if (c1.Value is TimeSpan c1ts && c2.Value is TimeSpan c2ts)
    return new LambdaParameterWrapper(c1ts + c2ts, c1.Cmp);
if (c1.Value is DateTime c1dt && c2.Value is TimeSpan c2ts)
    return new LambdaParameterWrapper(c1dt.Add(c2ts), c1.Cmp);

Also, all this cases should be covered with tests.

If you don't want / don't have time to make all these improvements, just create an issue and I'll add support of TimeSpan / DateTime on occasion.

some1one commented 5 years ago

Okie dokie, I will remove the rest and just implement the date time stuff.

some1one commented 5 years ago

Alright, that should do it

VitaliyMF commented 5 years ago

Nice job, thanks!