SmartFormat icon indicating copy to clipboard operation
SmartFormat copied to clipboard

Add extension to evaluate mathematical expressions inside a format string

Open axunonb opened this issue 2 years ago • 22 comments

Add a math formatter extensions that allows the following:

Smart.Default.FormatterExtensions.Add(new MathFormatter());
var data = new { Arg1 = 3, Arg2 = 4 };
_ = Smart.Format("{:M():({Arg1} + {Arg2}) * 5}");
// result: 35

NCalc might be a good candidate for a Mathematical Expressions Evaluator.

axunonb avatar Jul 18 '22 15:07 axunonb

@karljj1 @Begounet There is a branch for a new NCalcFormatter extension which integrates the NCalc package. The NCalcFormatter has as much as all features included, that NCalc can give. Still, I would call it an alpha.1. The unit tests give a good impression about features and capabilities. With some more extensions NCalcFormatter might in medium term replace ChooseFormatter and ConditionalFormatter. If time allows, please have a look. Some feedback would be great.

axunonb avatar Aug 08 '22 22:08 axunonb

I had not considered a math expression evaluator but it does sound like an interesting idea. Using it to replace the Choose and Conditional Formatter would be good. It may be worth renaming it to something like MathFormatter so it is more descriptive unless you think users need to know that it's NCalc.

I noticed you do

#if NETSTANDARD2_1_OR_GREATER || NET6_0_OR_GREATER
                sb.Append(literalItem.AsSpan());
#else
                sb.Append(literalItem);

From what I can see we already have a lot of AsSpancode in other parts without this guard so it's probably not needed. We had to increase the minimum version of Unity for the new SmartFormat 3.x features because we only started supporting Span a few years ago. We also struggled with things like the file scoped namespaces which we don't support. I had to update all the files to use the old namespacing technique.

How does NCalc perform when compared against the ChooseFormatter and ConditionalFormatters? How does it compare with memory, does it allocate much?

karljj1 avatar Aug 09 '22 09:08 karljj1

@karljj1 The latest version of the branch reflects your comments:

  • Renamed to LogicCalcFormatter
  • Removed preprocessor directives
  • Some performance / GC optimizations

Performance: The additional step of evaluating Placeholders in the format argument before the final evaluation by NCalc has its price. On the other side we get a bunch of new features, that none of the other formatters contains.

Still not sure, whether LogicCalcFormatter should become another extension. What do you think?

The Performance project of the branch contains a BenchmarkDotNet test, bringing the following results:

// * Summary *

BenchmarkDotNet=v0.13.0, OS=Windows 10.0.22000
AMD Ryzen 9 3900X, 1 CPU, 24 logical and 12 physical cores
.NET SDK=6.0.400
  [Host]   : .NET 6.0.8 (6.0.822.36306), X64 RyuJIT
  .NET 6.0 : .NET 6.0.8 (6.0.822.36306), X64 RyuJIT

Job=.NET 6.0  Runtime=.NET 6.0

|    Method |   N |       Mean |     Error |    StdDev |   Gen 0 | Gen 1 | Gen 2 | Allocated |
|---------- |---- |-----------:|----------:|----------:|--------:|------:|------:|----------:|
|      Cond |  10 |   7.273 us | 0.0950 us | 0.0889 us |  0.3204 |     - |     - |      3 KB |
|    Choose |  10 |   8.368 us | 0.0806 us | 0.0754 us |  0.5493 |     - |     - |      5 KB |
| LogicCalc |  10 |  16.688 us | 0.2579 us | 0.2286 us |  1.0986 |     - |     - |      9 KB |
| PureNCalc |  10 |   4.224 us | 0.0781 us | 0.0692 us |  0.7706 |     - |     - |      6 KB |
|      Cond | 100 |  72.114 us | 1.0332 us | 0.9665 us |  3.1738 |     - |     - |     27 KB |
|    Choose | 100 |  87.418 us | 1.2041 us | 1.1263 us |  5.4932 |     - |     - |     45 KB |
| LogicCalc | 100 | 167.624 us | 1.8362 us | 1.6277 us | 10.9863 |     - |     - |     91 KB |
Pure evaluation by NCalc:
| PureNCalc | 100 |  40.607 us | 0.8121 us | 1.1903 us |  7.6904 |     - |     - |     63 KB |

// * Hints *
Outliers
  LogicCalcTests.LogicCalc: .NET 6.0 -> 1 outlier  was  removed (17.47 us)
  LogicCalcTests.PureNCalc: .NET 6.0 -> 1 outlier  was  removed (4.43 us)
  LogicCalcTests.LogicCalc: .NET 6.0 -> 1 outlier  was  removed (173.20 us) => first time compilation

axunonb avatar Aug 20 '22 21:08 axunonb

Looks interesting. I think keeping it as its own extension makes sense, still, keep the existing Cond and Choose extensions as they seem lighter to use for both perf and allocations.

karljj1 avatar Aug 25 '22 08:08 karljj1

After evaluating the following OSS solutions

the best choice in terms of Smart.Format still seems to be NCalc

axunonb avatar Dec 05 '22 22:12 axunonb

How is this feature coming along?

Flithor avatar Nov 01 '23 02:11 Flithor

Actually I'm not yet satisfied with the current implementation, which is rather a POC. The response from SmartFormat users has been very low so far.

axunonb avatar Nov 01 '23 08:11 axunonb

@axunonb Currently I'm trying to write a "custom formula & format string" and this library is most fit for me on "format" feature, but it doesn't support "formula" now, so I have to do some magic to achieve what I need.

Flithor avatar Nov 01 '23 10:11 Flithor

@Flithor In this case you should give LogicCalcFormatter a try. Just check-out the pr-ncalc-formatter branch. It's alive and rebased to the latest release. I'd appreciate hearing your comments. Have a look at the SmartFormat.Tests/Extensions.LogicCalc/LogicCalcFormatterTests.cs to find out its usage.

axunonb avatar Nov 02 '23 11:11 axunonb

Just updated NCalc package reference

  • NCalcSync v3.8.0
  • Brutal.Dev.StrongNameSigner v3.4.0

axunonb avatar Nov 02 '23 13:11 axunonb

I was looking into using this, and it looks very cool, but since it requires changes into the core package and internal classes or a custom build it is really difficult to fit into the toolchain. It does not look like it easily could be an external extension?

petero-dk avatar Mar 11 '24 00:03 petero-dk

@petero-dk @Flithor Currently this is a proof of concept. The necessary changes to the core package must still be be reviewed and streamlined. ncalc is actively developed and quite powerful. It does not come with signed assemblies, though. We're not happy about using Brutal.Dev.StrongNameSigner, because it might complicate deployment. This is the main reason why the package got stuck. Do you have an idea how to overcome these obstacles?

axunonb avatar Mar 14 '24 20:03 axunonb

I built it and directly referenced the assemblies. Not really happy with that.

If you would like I can make a PR for the core with changes so the plugin can be completely in a different repository. But it will require some changes as it uses protected methods.

That way it can use an official core and we/you can create a pre release version of the calc plugin.

I think that would be a clean way of doing it and also show the community that it is not supported but allow usage for brave souls.

petero-dk avatar Mar 15 '24 07:03 petero-dk

@petero-dk Please let's first discuss how to deal with the ncalc assembly being unsigned:

  • Disassemble and reassemble the unsigned Assembly?
  • Load ncalc dynamically?
  • Use source code directly?

axunonb avatar Mar 18 '24 20:03 axunonb

Hi @axunonb

I spent a lot of time thinking about this, and I think the thing I keep coming back to is why the need for the strong name assembly? I can see that it is used to provide private access for testing and plugins, but I don't like that at all. Because that allows some plugins to have more capabilities than others and does not make it easy for other to expand upon it.

And then comes the point of the whole thing, no matter what we do with ncalc, it is a workaround for the strong naming, and it should not really be required for a plugin based system, especially one where we would like to utilize other nuget packages.

Then I may assume that the strong name is required upstream by some already, so it might be a non-negotiable thing, however as I read it, it has no real impact in .Net 5 and after (except for the private access)

What I would suggest "to start with" is to make everything needed in the SmartFormat base library public so even non-signed plugins can use them. The strong naming should not be a hindering for development, which it is now.

Then I would build the plugin completely separate without need for signing, because if ncalc does not come signed, then there are only two correct ways of handling it:

  • Find something that does the same but is signed,
    • (another library, use the source code directly internally (BAD), or
    • develop the internals ourselves (not really worth the effort)
  • Have this plugin be unsigned (which I would argue for)

petero-dk avatar Mar 27 '24 11:03 petero-dk

Thank you, @petero-dk, for sharing your thoughts on the necessity of strong-naming the SmartFormat NuGet package and its implications, especially in the context of accommodating unsigned packages like NCalc.

Your points are valid, and indeed, strong naming does present challenges, particularly in plugin-based systems where interoperability with other NuGet packages is advantageous. On the other hand, publishing SmartFormat unsigned would present a number of the current users with the same problem that SmartFormat has with the NCalc package.

I did examine other packages closely, but came back to NCalc as the best fit.

Following your 'unsigned proposal', a pragmatic approach could be, to limit the strong-named LogiCalc extension for targeting .NET 6.0 (LTS) and above, where strong-named dependencies are not mandatory. Here we can suppress the CS8002 warning for strong-named assemblies referencing non-strong named assemblies.

axunonb avatar Mar 28 '24 13:03 axunonb

Actually I think my unsigned proposal would be to split out the Calculator plugin in its completely own repository and Nuget package, and keep that completely unsigned and for all targets.

That would would allow everybody to use it, but obviously if people are using below .Net 6 they would have to be unsigned themselves.

Keep the main repository as is and signed.

This would be completely backwards compatible.

petero-dk avatar Mar 29 '24 07:03 petero-dk

@petero-dk @Flithor Just submitted a PR to publish a signed version besides the unsigned version of NCalcSync. Hopefully, it will be accepted.

axunonb avatar Apr 27 '24 14:04 axunonb

@petero-dk @Flithor The PR for a signed version of NCalcSync has been merged today 👍

axunonb avatar May 03 '24 06:05 axunonb