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

Add extension to evaluate mathematical expressions inside a format string #299

Open
axunonb opened this issue Jul 18, 2022 · 19 comments
Open

Comments

@axunonb
Copy link
Member

axunonb commented Jul 18, 2022

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 axunonb changed the title Add extension to evaluate mathmatical expressions inside a format string Add extension to evaluate mathematical expressions inside a format string Jul 18, 2022
@axunonb
Copy link
Member Author

axunonb commented Aug 8, 2022

@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.

@karljj1
Copy link
Collaborator

karljj1 commented Aug 9, 2022

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?

@axunonb
Copy link
Member Author

axunonb commented Aug 20, 2022

@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

@karljj1
Copy link
Collaborator

karljj1 commented Aug 25, 2022

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.

@axunonb
Copy link
Member Author

axunonb commented Dec 5, 2022

After evaluating the following OSS solutions

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

@Flithor
Copy link

Flithor commented Nov 1, 2023

How is this feature coming along?

@axunonb
Copy link
Member Author

axunonb commented Nov 1, 2023

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.

@Flithor
Copy link

Flithor commented Nov 1, 2023

@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.

@axunonb
Copy link
Member Author

axunonb commented Nov 2, 2023

@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
Copy link
Member Author

axunonb commented Nov 2, 2023

Just updated NCalc package reference

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

@petero-dk
Copy link

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?

@axunonb
Copy link
Member Author

axunonb commented Mar 14, 2024

@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?

@petero-dk
Copy link

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.

@axunonb
Copy link
Member Author

axunonb commented Mar 18, 2024

@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?

@petero-dk
Copy link

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)

@axunonb
Copy link
Member Author

axunonb commented Mar 28, 2024

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.

@petero-dk
Copy link

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.

@axunonb
Copy link
Member Author

axunonb commented Apr 27, 2024

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

@axunonb
Copy link
Member Author

axunonb commented May 3, 2024

@petero-dk @Flithor The PR for a signed version of NCalcSync has been merged today 👍
and is published now: NuGet

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

No branches or pull requests

4 participants