-
Notifications
You must be signed in to change notification settings - Fork 95
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
support for decimal #12
Comments
It is possible, but it has quite some impact. |
As I would like to use Jace for calculations involving money, decimal support is really important for me as well. I've begun with a really naive port of Jace where I simply cloned the project and started replacing "double" with "decimal" whenever I ran into it, but you quickly run into the code that emits the IL and there is no trivial way to replace the IL for doubles with IL for decimals because double is supported natively in IL while decimal is not so the IL is much more complicated (just emitting a constant is much harder). Maybe it's because I'm not very familiar with IL emission, but the easiest solution for me to this would be to use the .NET Expression API, which does natively support decimal, and compile an expression to a dynamic method. I also don't know if it's feasible to make this change while also keeping double support. To me it seems like that wouldn't be really possible without introducing a secondary EDIT: I see the |
I will consider the support of adding decimal. But I need to investigate a clean way to offer both double and decimal support through the API. If you have an idea, please let me know. It is not an issue to support the necessary IL code. So don't worry about that aspect :-) |
Well, it might be nice to do something with generics. The public API of CalculationEngine can easily be made generic: public interface ICalculationEngine<T>
{
void AddConstant(string constantName, T value);
void AddFunction(string functionName, Func<T, T, T, T, T, T, T> function);
void AddFunction(string functionName, Func<T, T, T, T, T, T> function);
void AddFunction(string functionName, Func<T, T, T, T, T> function);
void AddFunction(string functionName, Func<T, T, T, T> function);
void AddFunction(string functionName, Func<T, T, T> function);
void AddFunction(string functionName, Func<T, T> function);
void AddFunction(string functionName, Func<T> function);
Func<System.Collections.Generic.Dictionary<string, T>, T> Build(string formulaText);
T Calculate(string formulaText);
T Calculate(string formulaText, System.Collections.Generic.IDictionary<string, T> variables);
Jace.Execution.FormulaBuilder Formula(string formulaText);
} Of course, for backwards compatibility, you still want public static class CalculationEngineFactory
{
public static ICalculationEngine<T> Create<T>()
{
..
}
} |
Just a quick update on this. I went ahead and tried to make Jace work with both double as well as decimal. This is by no means a "please merge this" yet (it's a pull request for my own fork anyway ;)), more of a proof of concept. For the most part, it was a matter of turning classes that depended on Using the decimal support now involves creating a
Since they would differ only by return type, which wouldn't work. For our own internal purposes for Jace, my changes are 'good enough' but I'd be happy to work on making it mergeable back to master. There's probably some unnecessary code duplication still, especially the decimal interpreter is just a copy-paste of the double interpreter. I'll see if I can unify these again. Because there's no generics support for operators this would require adding an abstraction for additional/multiplication/etc, marginally slowing down the interpreter. |
Thanks for the suggested modfications. I will have a more detailed look at your code this weekend. I like the idea, I only don't like the code duplication... ;-) |
Agreed, so I just updated the code ;) I got rid of the duplication with the interpreter by doing what I said in my last comment. Also, I added some comments to the pull request to clarify/justify some of the code I've changed. My eye also caught this other issue that was opened: It's probably pretty trivial to add BigInteger support to Jace now. The only real hurdle I see is the IL emit support. I don't want to dictate how you write your code of course, but isn't the Expression API pretty widely available on platforms now and couldn't you make it the default instead of IL emit and use that as a fallback only? Just an idea :) |
Have you had a chance to look over my suggested changes yet? :) Like I said, for us the changes I've made work perfectly fine and are 'good enough', but of course it'd be nice if they could some day be contributed back to master. Additionally, in the meantime I've mostly implemented BigInteger support as a proof of concept in very few lines of code and in a single file and it seems to work fine (aside from parts of the expression generator that rely on Again, that's leaving aside the IL generation bits, which are just too scary for me ;) |
Sorry for the (very) late response :-) We should pull your changes back in the master of Jace. Support for decimal and BigInteger would be an interesting feature for Jace. And don't be afraid of the IL generation. I think we could better drop it. It is only required for older platforms such as Windows Phone 7. And support for these platforms can be easily dropped. So we can simplify the source code of Jace by doing that. |
Only just noticed your response! Sorry for my late response now ;) That'd be great! I'll try to make my code ready for a pull request in the next few days. The BigInteger code isn't ready yet, but the decimal code is. I'll try to add tests for it first though. |
Can you please add this feature. We are still waiting for it :) |
@rcl2748 the need for Jace in our application got put on the backburner, so never got around to creating a proper pull request for this, but if creating a custom build is an option for you (rather than waiting for an updated NuGet package) then my branch contains fully working code for decimal support: https://github.com/JulianRooze/Jace/tree/feature/decimal-support Just clone and build and you should be good to go. |
Hi, any progress on decimal support? I've tried using @JulianRooze branch, but unfortunately some tests are failing on this branch.
With test result:
|
Hello @mesika I could look to bring proper support to the official version of Jace. But it was never a high priority for me, because double was good-enough for me. In which context would you require decimal support? |
Thanks @pieterderycke I appreciate it. |
Hi @mesika , Anyway, thank you @pieterderycke for your library. |
Hi! Like Julian I need decimal support. So I've just updated Julian's solution of decimal's support. All doubles tests are passing but need to create some tests for decimals calculations too. https://github.com/mrxrsd/Jace/tree/feature/decimal-support-updated I forgot to updated TokenReader, decimals version is not working yet. |
100% passed in all tests (double and decimal). I've replicated all double's tests with the new decimal engine. |
@mrxrsd thanks for the work on supporting Decimal - we'd really like to have this vetted and moved to the main build of Jace. Seeing as how it was originally called out as going to be supported with 1.0, is there any ETA on a proper merge and release with decimal support? @pieterderycke |
You can check my repository: https://github.com/mrxrsd/jace This feature and others fixes are merged there but I never publish a nuget package by my own. |
@pieterderycke
What effort, would you guess, is required to get Jace to support decimal (not just double)?
Evan
The text was updated successfully, but these errors were encountered: