Add Dependency Injection support #160
Conversation
|
@Bykiev ready for review. Still needs docs, it's 11pm here, time for some sleep :) |
|
|
||
| public required Dictionary<string, object?> Parameters { get; set; } |
There was a problem hiding this comment.
@Beckdotan this PR will also solve your problem.
| public static implicit operator ExpressionContext(ExpressionOptions options) | ||
| { | ||
| return new ExpressionContext { Options = options }; | ||
| } | ||
|
|
||
| public static implicit operator ExpressionContext(CultureInfo cultureInfo) | ||
| { | ||
| return new ExpressionContext { CultureInfo = cultureInfo }; |
There was a problem hiding this comment.
Just context for reviewers, this prevent a breaking change in the Expression constructors (the proof is the unit tests are untouched).
It's what Microsoft do with LocalizedString->string at Microsoft.Extensions.Localization with IStringLocalizer<T>
|
Great job, I'll review it carefully at evening. These changes seems too complex and breaking, I'd say it's for NCalc v5. What do you think? |
I'm in doubt, if we were to follow semver, it would really be v5, but I consider the breaking change nearly irrelevant, I think is very hard someone using Ohhh, wait, typing this I just discovered how we workaround and not have breaking changes, maybe I can just add So, without breaking changes, I vote in 4.1. |
|
Now No Breaking Changes, in a v5 we can remove the class (also added |
There was a problem hiding this comment.
Looking good, can you please add some plugin tests? Also we need to update the docs. When you'll merge, can you please create a separate branch, not in the master. I think this feature will be ready for production in NCalc v4.2 and v4.1 will include bug fixes and a new features - semicolon as argument delimeter and compare with null parameters
# Conflicts: # src/NCalc/Parser/LogicalExpressionParser.cs
|
@Bykiev ready to merge. But I vote in a single release of 4.1, it will be maintenance hell of both 4.1 an 4.2 with different feats I think. This PR does not introduce any breaking changes, and I already wants to use this at my project. |
|
Finished my self-review, LGTM now. |
|
Agreed, as there is no any breaking changes, let's include it in v4.1. Don't see any issues, can be merged |
This closes #154
This closes #163
This closes #148