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

Allow decimals to be used as hints for converting to floats #20

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

builtbybrayne
Copy link

Two new options for strategies for converting to floats.

The problem I had was that a number like '1.234567890123456789' is longer than 15 characters and therefore was being converted to a BigNumber. In my case, I definitely want this left as a float and don't care about the loss of precision.

To this end I created 2 options for parsing numbers as floats: options.floatHints and options.strictFloatHints. Both are documented in the updated readme. But long story short; the strict version always treats the presence of a decimal as an instruction to parse the number as a float. The loose version will only parse the number as a float if the mantissa is non-zero. If it is zeroable, then it is stripped, with the integer part then subject to the same logic that existed previously.

Tested.

@sidorares
Copy link
Owner

the PR looks ok, but I'm a bit worried about growing nimber of options like yours. Maybe we should move to much simple api, for example toNumber() callback that takes number-like string as an input and decides what to return - JS number, BigNumber or something else. If not present the behaviour is automatic ( BigNumber if not all integer digits can be preserved accurately )

@diyijing
Copy link

diyijing commented Nov 1, 2018

@sidorares why not merge this PR ?

@AndKiel
Copy link

AndKiel commented Oct 23, 2020

This bug with parsing very long decimal numbers is making the package unusable for people who want to stick to the built-in number/bigint instead of using bignumber package. :/

@arieb
Copy link

arieb commented May 23, 2021

@sidorares any chance this can be merged and a new minor released?

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.

5 participants