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

mersenne.rand without min argument returns NaN #479

Closed
piotrekn opened this issue Feb 12, 2022 · 5 comments · Fixed by #577
Closed

mersenne.rand without min argument returns NaN #479

piotrekn opened this issue Feb 12, 2022 · 5 comments · Fixed by #577
Assignees
Labels
c: bug Something isn't working p: 1-normal Nothing urgent s: accepted Accepted feature / Confirmed bug

Comments

@piotrekn
Copy link
Contributor

piotrekn commented Feb 12, 2022

Describe the bug

This is buggy, cause if min is not passed but only max, then min will be undefined and this result in NaN for the whole function

faker/src/mersenne.ts

Lines 31 to 40 in 30b0faa

rand(max?: number, min?: number): number {
// TODO @Shinigami92 2022-01-11: This is buggy, cause if min is not passed but only max,
// then min will be undefined and this result in NaN for the whole function
if (max === undefined) {
min = 0;
max = 32768;
}
return Math.floor(this.gen.genrand_real2() * (max - min) + min);
}

Reproduction

new Mersenne().rand(1);

This should return 0 no matter the seed, but returns NaN.

Additional Info

No response

@piotrekn piotrekn added the s: pending triage Pending Triage label Feb 12, 2022
@ST-DDT ST-DDT added c: bug Something isn't working and removed s: pending triage Pending Triage labels Feb 13, 2022
@import-brain import-brain added the p: 2-high Fix main branch label Feb 13, 2022
@Shinigami92 Shinigami92 added this to the v6.1 - First bugfixes milestone Feb 13, 2022
piotrekn added a commit to piotrekn/faker that referenced this issue Feb 14, 2022
@xDivisionByZerox
Copy link
Member

I want to do this. Please assign me.

@xDivisionByZerox
Copy link
Member

I also implemented an if statement that checks if min > max in which case it throws an error. This behavior is breaking 83 test in 4 suits.

What would the expected behavior be here?

@ST-DDT
Copy link
Member

ST-DDT commented Feb 23, 2022

I also implemented an if statement that checks if min > max in which case it throws an error. This behavior is breaking 83 test in 4 suits.

What would the expected behavior be here?

Not sure on this one. I tend to allow min and max to be swapped as a kind of undocumented convenience feature.

@piotrekn
Copy link
Contributor Author

If mersenne.rand does not bother the min-max swap, should it thrown an error? To maintain the semantic meaning consider swapping values instead.

@xDivisionByZerox
Copy link
Member

FYI: Swapping the values will not break any existing test cases.

@ST-DDT ST-DDT linked a pull request Feb 27, 2022 that will close this issue
@Shinigami92 Shinigami92 added p: 1-normal Nothing urgent and removed p: 2-high Fix main branch labels Mar 15, 2022
@ST-DDT ST-DDT added the s: accepted Accepted feature / Confirmed bug label Mar 18, 2022
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 p: 1-normal Nothing urgent s: accepted Accepted feature / Confirmed bug
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants