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

fix: decimal javascript calculation #555

Conversation

franznoel
Copy link

@franznoel franznoel commented Feb 24, 2022

Description

  • Fix decimal Javascript calculation using decimal.js library
  • Added unit test for precision values 0.00001 and 0.000000001

closes #331

@franznoel franznoel requested a review from a team as a code owner February 24, 2022 07:53
@Shinigami92
Copy link
Member

I would like to discuss with the maintainer team internally if dependencies should be really needed for faker or if we want to try to mostly not rely on dependencies to keep the size of faker as low as possible.

@ejcheng ejcheng added c: bug Something isn't working p: 2-high Fix main branch labels Feb 24, 2022
@Shinigami92 Shinigami92 added c: dependencies Pull requests that adds/updates a dependency p: 1-normal Nothing urgent s: needs decision Needs team/maintainer decision and removed p: 2-high Fix main branch labels Mar 15, 2022
@ejcheng ejcheng added this to the v6.1 - First bugfixes milestone Mar 19, 2022
@Shinigami92 Shinigami92 changed the title fix: decimal javascript calculation (close #331) fix: decimal javascript calculation Mar 23, 2022
@Shinigami92
Copy link
Member

Yesterday we discussed in a maintainers team meeting that we want to go with an optional peer dependency and a log message that can be somehow toggled on and off
So when someone wants precise correct decimal values, they have the option to additionally install decimal.js and then benefit from the "better" variant
And if they don't need it, they will just get the value / variant that is currently available

@Shinigami92 Shinigami92 added needs rebase There is a merge conflict and removed s: needs decision Needs team/maintainer decision labels Mar 25, 2022
@Shinigami92
Copy link
Member

One week passed without response from the author, it may be that this feature will fall out of the v6.1 milestone and will be just handled when it's done

@prisis
Copy link
Member

prisis commented Apr 2, 2022

Should someone take if over?

@Shinigami92
Copy link
Member

Should someone take if over?

I could take it over, but later
I'm a bit interested into how we handle optional peer dependencies, but it doesn't feel like it needs to be tackled immediately, so I focus my time for more important stuff right now

@xDivisionByZerox
Copy link
Member

I will close this due to inactivity and #779.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c: bug Something isn't working c: dependencies Pull requests that adds/updates a dependency needs rebase There is a merge conflict p: 1-normal Nothing urgent
Projects
None yet
Development

Successfully merging this pull request may close these issues.

datatype.number does not always generate numbers with expected precision
6 participants