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

Missing parse from binary literal to integer #552

Closed
RafaelEnriquez opened this issue Nov 10, 2021 · 6 comments
Closed

Missing parse from binary literal to integer #552

RafaelEnriquez opened this issue Nov 10, 2021 · 6 comments
Assignees
Labels

Comments

@RafaelEnriquez
Copy link

ExpressionParser parses from hexadecimal literal to integer, but is missing a parse from binary literal to integer.

https://dotnetfiddle.net/cTfqh7

Exception

@StefH StefH self-assigned this Nov 13, 2021
@StefH
Copy link
Collaborator

StefH commented Nov 18, 2021

#554

@StefH
Copy link
Collaborator

StefH commented Nov 18, 2021

Hello @RafaelEnriquez ;

I think the linked PR should solve this, if you have time, can you take a look?

@StefH StefH added the bug label Nov 18, 2021
@RafaelEnriquez
Copy link
Author

Hi @StefH , thanks for your work on this.

I was expecting the fix to be something like the existing code for hexadecimal in "ExpressionParser.ParseIntegerLiteral".

Does this PR makes the provided fiddle test pass???

Regards

@StefH
Copy link
Collaborator

StefH commented Nov 24, 2021

Hello @RafaelEnriquez,

I did move some code from ExpressionParser.ParseIntegerLiteral to a new file and also added some more tests for that new code.

See
https://github.com/zzzprojects/System.Linq.Dynamic.Core/blob/stef-binary/test/System.Linq.Dynamic.Core.Tests/Parser/NumberParserTests.cs#L137

(I think this covers your scenario, but if you have questions or want to add more test, please let me know)

@StefH
Copy link
Collaborator

StefH commented Dec 1, 2021

Hello @RafaelEnriquez, did you have time to check if the unit-test is enough for your scenario?

@RafaelEnriquez
Copy link
Author

Hi @StefH . Yes, the tests you added are enough.

Thank you very much.

@StefH StefH closed this as completed Dec 2, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

No branches or pull requests

2 participants