Skip to content
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 DateTime and Timespan support #17

Merged
merged 6 commits into from
Mar 29, 2019
Merged

Conversation

some1one
Copy link
Contributor

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

@VitaliyMF
Copy link
Contributor

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 Improve performance by compiling into lambda method calls, property getters etc #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
Copy link
Contributor Author

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

@some1one some1one changed the title added delegate creation, force caching on/off for a particular evalua… Adding DateTime and Timespan support Mar 29, 2019
@some1one
Copy link
Contributor Author

Alright, that should do it

@VitaliyMF VitaliyMF merged commit 87fed74 into nreco:master Mar 29, 2019
@VitaliyMF
Copy link
Contributor

Nice job, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants