-
Notifications
You must be signed in to change notification settings - Fork 194
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
#25 parse very big numeric values #26
base: master
Are you sure you want to change the base?
#25 parse very big numeric values #26
Conversation
…of string.length to ensure scientific notations like i.e 3e500 will also be parsed to BigNumber objects
…623157e+308 will be parsed correctly
Hi, do you already have some feedback regarding this pull request? Will appreciate if you can merge my changes soon as I need this feature now. |
- adjust name and url - remove author to avoid conflicts with the original author
…cientific notation
Hi, I just wanted to let you know that I published my fork at npm (https://www.npmjs.com/package/true-json-bigint) as I now really need these changes. Of course, I also referenced your project in the README.md. I hope you don't mind. If I find some time I maybe will keep on extending it by i.e. adding some more test cases etc. It still will be nice if you find some time to merge my pull request. |
…arize the purpose of this fork
…eaking change in chai
…ocally on windows cmd
Add typescript typings
Hightlight javascript code in examples
…for consistency reasons
…mocha 7.1.1 expects nodeJS version >=8.0.0
Hi @SebastianG77 sorry it's been unanswered for so long. Happy to see you change here. Would you be able to rebase? |
Hi, yes, I generally can do so. Meanwhile you did quiet a lot of changes. Since I am currently quiet busy, I will not be able to have a look at it the next days. So give me a little while until I will have time for it. However, meanwhile I also merged a pull request of @xamgore and also did some minor changes regarding CI, dependency versions and refactoring in my fork. I also adapted the documentations, but I of course would prefer yours after merging. See my commit history for further details: https://github.com/SebastianG77/true-json-bigint/commits/master. Would be glad to also merge these changes into your repository so there will just be one source to maintain. |
…d renew yarn.lock
…se mocha 8.1.1 expects nodeJS version >= 10.12.0
# Conflicts: # .travis.yml # README.md # lib/parse.js # package.json # test/strict-option-test.js
…ng this module but not yarn
Hi @sidorares, I finally have just updated my PR. Since my changes are almost two years old now, a lot has changed and I had to be very careful with merging your changes. I still think everything is working fine now, but please also have a detailed look on this PR. It also considers all my other changes and I will be happy if you are able to integrate them too. So in addition to the main changes of my first post this PR also considers the following ones:
|
I forgot to mention: While testing the merged version, I figured out it is not possible to use scientific notation while having option "useNativeBigInt: true" activated. In this case an error will be thrown now. In your previous versions this issue has been veiled as values written in scientific notation have usually always been handled as ordinary numbers but not as big numbers. However, when activating both options "useNativeBigInt: true" and "alwaysParseAsBig: true" this problem also already occurs in version 1.0.0. I do not think this issue can be fixed here as it obviously directly relates to BigInt, but I still wanted to let you know so you will not be scared if you also bump into the same problem. |
can you post small example illustrating this? |
Sure, but I have realized I was a bit too overhasty and figured out setting "alwaysParseAsBig: true" will have no effect in this context. However, you still can construct this this issue with version 1.0.0, but you then need to avoid the "Bad Number" exceptions. So this example will work as the value of "bigNumber" has 15 characters:
This example will not work as "bigNumber" has more than 15 characters, such that the BigInt Object will be created with a string value, but not with a numeric one.
On the other hand this example would also work, as here we are using bignumber.js Objects.
Anyway, while building a suitable example, I figured out that with this PR the following example will work:
But this one will still return a "Bad Number" Exception:
The reason for that is that I replaced function Number.isFinite() by BigNumber.isFinite(). According to the API-documention, this function should only return false if values NaN, Infinity or -Infinity are checked (https://mikemcl.github.io/bignumber.js/#isF). However, obviously this function also returns Infinity if one uses very big BigNumber-Objects like 1e12345678. I would still like to leave it as it is for now, because even though we still will get some undesired "Bad Number" Exception, this PR reduces the cases that will lead to such an exception. |
It is a bit late now, but I have fixed the example in my previous comment as the original one did not illustrate the problem I wanted to show you. |
Exchanged
if (!isFinite(number))
byif (!bigNumberValue.isFinite())
and had to move this function a few lines down in this context, but I think this check is not necessary at all, because whenever a numeric value will be entered, it must be finite anyway.Replaced
if (string.length > 15)
byif (bigNumberValue.toFixed().length > 15)
to ensure big numbers written in scientific notation will also be parsed to BigNumber objects.Added a test case for evaluating my changes.