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

Accept mix casing exponential notation #16

Merged
merged 1 commit into from
Mar 28, 2023
Merged

Accept mix casing exponential notation #16

merged 1 commit into from
Mar 28, 2023

Conversation

tiagox
Copy link
Contributor

@tiagox tiagox commented Mar 27, 2023

Exponential notation in JavaScript accept e and E for the exponential part of the number. Since you're relying on from-exponential which also accepts any casing for the syntax of a number written in exponential notation, I believe it's correct to accept this too, when converting a string into a BigNumber.

While using toBn on our project we discovered that a number like '1.2345E+20' was throwing: Unknown format for fixed-point number: 1.2345E+20

Our workaround was to change that E with an e but this seems a little bit hacky and it's probably better for toBn to support this case.

Exponential notation in JavaScript accept `e` and `E` for the exponential part of the number. Since you're relying on `from-exponential` which also accepts any casing for the syntax of a number written in exponential notation, I believe it's correct to accept this too, when converting a string into a BigNumber.
@PaulRBerg
Copy link
Owner

PaulRBerg commented Mar 27, 2023

Thanks for the PR, @tiagox. Happy to accept this, but before we merge the PR, could you write some tests, please?

Sorry for the question above. For some reason, I missed your tests.

I will review this later this week.

@tiagox
Copy link
Contributor Author

tiagox commented Mar 27, 2023

Thanks for the PR, @tiagox. Happy to accept this, but before we merge the PR, could you write some tests, please?

Sure, happy to add more tests, is there any case you'd like to cover? I've updated one of the tests you've already had.

(I've pushed a couple of time to this branch, sorry I've created to PR before adding tests and checking that everything work)

@PaulRBerg
Copy link
Owner

I've updated one of the tests you've already had.

Yeah, I see that now! Sorry. Missed them before.

Copy link
Owner

@PaulRBerg PaulRBerg left a comment

Choose a reason for hiding this comment

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

LGTM

@PaulRBerg PaulRBerg merged commit 68f2c37 into PaulRBerg:main Mar 28, 2023
Repository owner deleted a comment from coveralls Mar 28, 2023
@PaulRBerg
Copy link
Owner

Included in v1.2.0.

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

Successfully merging this pull request may close these issues.

2 participants