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

feat: precise number calculation #779

Closed
wants to merge 5 commits into from

Conversation

Shinigami92
Copy link
Member

closes #331

supersedes #555

@Shinigami92 Shinigami92 added c: feature Request for new feature p: 1-normal Nothing urgent c: dependencies Pull requests that adds/updates a dependency labels Apr 5, 2022
@Shinigami92 Shinigami92 added this to the v6.2 - New small features milestone Apr 5, 2022
@Shinigami92 Shinigami92 self-assigned this Apr 5, 2022
@codecov
Copy link

codecov bot commented Apr 5, 2022

Codecov Report

Merging #779 (b209d33) into main (1885dd0) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main     #779   +/-   ##
=======================================
  Coverage   99.34%   99.34%           
=======================================
  Files        1922     1922           
  Lines      183057   183064    +7     
  Branches      903      904    +1     
=======================================
+ Hits       181860   181867    +7     
  Misses       1141     1141           
  Partials       56       56           
Impacted Files Coverage Δ
src/datatype.ts 100.00% <100.00%> (ø)

@Shinigami92
Copy link
Member Author

It looks like that this is not healthy and when using somehow e.g. rollup to bundle your app it fails

image

So instead of using a peerDependency with optional, I'm thinking of providing an option to pass a precision calculation function to faker or number module 🤔

We could e.g. do this:

import { faker } from '@faker-js/faker'
import Decimal from 'decimal.js'

faker.Decimal = Decimal

Or maybe even make it more general like

import { faker } from '@faker-js/faker'
import Decimal from 'decimal.js'

faker.datatype.number({
  min,
  max,
  precision: 0.00001,
  precisionAlgo: (precision) => new Decimal(1)
        .dividedBy(precision)
        .toNumber()
})

But this has the problem that it could drastically compromise the value by just using a wrong implementation.
But beside that, I personally prefer the first solution due to you only need to set it once and for all application wide.

@ST-DDT
Copy link
Member

ST-DDT commented Apr 5, 2022

We could e.g. do this:

import { faker } from '@faker-js/faker'
import Decimal from 'decimal.js'

faker.Decimal = Decimal

We should not pollute our public API like this.

  precisionAlgo: (precision) => new Decimal(1)
        .dividedBy(precision)
        .toNumber()

Hard no.

src/datatype.ts Outdated
import type { Faker } from '.';
import { deprecated } from './internal/deprecated';

/**
* Module to generate various primitive values and data types.
*/
export class Datatype {
private Decimal: typeof Decimal | null = null;
Copy link
Member

@ST-DDT ST-DDT Apr 5, 2022

Choose a reason for hiding this comment

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

IMO this should be a function (value, precision) => number or just a divide/multiply function.

Copy link
Member Author

Choose a reason for hiding this comment

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

So yeah, like a combination of both my suggestions, but at module-scope instead of faker-scope

Copy link
Member

Choose a reason for hiding this comment

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

@internal or private

Copy link
Member Author

@Shinigami92 Shinigami92 Apr 5, 2022

Choose a reason for hiding this comment

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

@internal or private

No, non of these, the user needs to be able optionally provide they own algorithm. That's the point of this PR.
Please have a look into my newest commit.

@@ -5,6 +5,10 @@ import { deprecated } from './internal/deprecated';
* Module to generate various primitive values and data types.
*/
export class Datatype {
public preciseNumberDivider:
Copy link
Member Author

Choose a reason for hiding this comment

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

Does someone have a better name?

I also need to declare and use it like this way, because if I eagerly provide a default function, it counts as datatype module function and will be bound by the constructor and the all_functional test fails badly.

@Shinigami92 Shinigami92 added needs documentation Documentations are needed needs test More tests are needed labels Apr 5, 2022
@Shinigami92 Shinigami92 added the s: awaiting more info Additional information are requested label May 4, 2022
@xDivisionByZerox xDivisionByZerox added the m: datatype Something is referring to the datatype module label Jul 28, 2022
@Shinigami92
Copy link
Member Author

Currently looks like there is no much interest in this PR, will close it for now
If someone else would like to tackle the original issue, feel free

@Shinigami92 Shinigami92 deleted the 331-datatype-number-exact-precision branch August 20, 2022 10:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c: dependencies Pull requests that adds/updates a dependency c: feature Request for new feature m: datatype Something is referring to the datatype module needs documentation Documentations are needed needs test More tests are needed p: 1-normal Nothing urgent s: awaiting more info Additional information are requested
Projects
None yet
Development

Successfully merging this pull request may close these issues.

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