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

Configurable default numeric type #146

Conversation

bortolin
Copy link
Contributor

Hi Davide,

The Parser correctly consider numbers as integers or doubles if they have decimal places just like the C# compiler does.

In some cases it may be useful to be able to configure the default type of numbers if no particular suffix is ​​specified: for example in financial calculations, where usually numbers are interpreted as decimal type.

Another user @MrGorki have write about this on this issue #35.

I have implemented DefaultNumberType settings for configure the default type of number (Int, Long, Float, Double, Decimal).

I have also update documentation about this new features to specify how to use.

I have not touch the default behavior of parser if the settings is not set or set to Default.

I hope this new feature can be useful to others who like me use this beautiful library in financial application.

Marco

@davideicardi
Copy link
Member

Thank you @bortolin ! I like the idea!

I will review the code in depth in the following days.

@metoule Given that you have recently touch that part of the code, can I ask your help on review it when you have some free time? Do you see problems?

@metoule
Copy link
Contributor

metoule commented May 26, 2021

I'd just add a few more tests, but otherwise it looks good to me :)

Copy link
Member

@davideicardi davideicardi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! I ask you two final changes. See my comments.

src/DynamicExpresso.Core/Interpreter.cs Outdated Show resolved Hide resolved
test/DynamicExpresso.UnitTest/LiteralsTest.cs Show resolved Hide resolved
Copy link
Member

@davideicardi davideicardi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great! Thank you!

@davideicardi davideicardi merged commit f505bd9 into dynamicexpresso:master May 29, 2021
@davideicardi
Copy link
Member

Published in version 2.6.0

@metoule
Copy link
Contributor

metoule commented Jun 8, 2021

I think this PR closes #35 ?

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

Successfully merging this pull request may close these issues.

3 participants