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(number): support step on faker.number.int() #2188

Closed
wants to merge 1 commit into from

Conversation

leon
Copy link

@leon leon commented May 30, 2023

Fixes #2186

In faker 7 I could specify

faker.datatype.number({ min: 30, max: 200, precision: 10 }) // 140, 90, 180, ...
faker.datatype.number({ min: 5000, max: 20000, precision: 100 }) // 5200, 18500, 12900,...

But with faker 8, the precision was removed from int.

faker.number.int({ min: 30, max: 200 })

Suggested solution

Allow passing a step to the int faker function

faker.number.int({ min: 30, max: 200, step: 10 })

@@ -91,11 +97,17 @@ export class NumberModule {
throw new FakerError(`Max ${max} should be greater than min ${min}.`);
}

if (step <= 0) {
throw new FakerError(`Step should be a positive number.`);
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we also throw on a noninteger step?

Comment on lines +108 to +110
const range = (effectiveMax - effectiveMin) / step;
const rand = Math.floor(real * (range + 1));
return effectiveMin + rand * step;
Copy link
Contributor

Choose a reason for hiding this comment

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

This does not work correctly.

For example faker.number.int({min:0, max:5, step: 2}) sometimes returns 6.

range is (5 - 0)/2 = 2.5
rand is say floor(0.99 * 3.5) = 3
returns 0 + 3 * 2 = 6

Copy link
Contributor

Choose a reason for hiding this comment

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

Also per #1855 (comment)

faker.number.int({min:5, max: 100, step: 10}) should return {10, 20, ...} not {5, 15, 25...}

@codecov
Copy link

codecov bot commented May 30, 2023

Codecov Report

Merging #2188 (f4dedf7) into next (27a8ff7) will increase coverage by 0.00%.
The diff coverage is 92.85%.

Additional details and impacted files
@@           Coverage Diff           @@
##             next    #2188   +/-   ##
=======================================
  Coverage   99.59%   99.59%           
=======================================
  Files        2607     2607           
  Lines      245029   245041   +12     
  Branches     1151     1152    +1     
=======================================
+ Hits       244034   244046   +12     
  Misses        968      968           
  Partials       27       27           
Impacted Files Coverage Δ
src/modules/number/index.ts 99.75% <92.85%> (-0.25%) ⬇️

... and 1 file with indirect coverage changes

@Shinigami92 Shinigami92 added s: needs decision Needs team/maintainer decision m: number Something is referring to the number module labels May 30, 2023
@Shinigami92 Shinigami92 added this to the v8.1 - Split Faker Class milestone May 30, 2023
@Shinigami92
Copy link
Member

Marked this for needs decision so we have time to discuss in a team meeting how the best way is to solve this and if the current ideas are the "right" way of doing it
The next (full/relevant) team meeting wont be before July 2023

@@ -163,6 +163,22 @@ describe('number', () => {
new FakerError(`No integer value between 2.1 and 2.9 found.`)
);
});

Copy link
Member

Choose a reason for hiding this comment

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

line 9 should be extracted and separated together with a new it line

@ST-DDT
Copy link
Member

ST-DDT commented Jan 18, 2024

Closed in favor of #2586

@ST-DDT ST-DDT closed this Jan 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
m: number Something is referring to the number module s: needs decision Needs team/maintainer decision
Projects
No open projects
Status: In Progress
Development

Successfully merging this pull request may close these issues.

Allow passing precision to faker.number.int
4 participants