fix(number): bigint shouldn't throw when max is negative

[soc221b]

No description provided.

[netlify]

✅ Deploy Preview for fakerjs ready!

Name	Link
🔨 Latest commit	5e4aa59
🔍 Latest deploy log	https://app.netlify.com/sites/fakerjs/deploys/6777f953afe8d50008f24e5e
😎 Deploy Preview	https://deploy-preview-3363.fakerjs.dev
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

---

To edit notification comments on pull requests, go to your Netlify site configuration.

[xDivisionByZerox]

I confirmed that this is a bug in the current version (v9.3.0) of Faker. Adding the acceped label.

[ST-DDT]

I'm not sure about it.
Afaict all our number methods return 0-max if only max is specified. So either we have to change all methods to behave like this or the user has to specify both min and max (and maybe we have to adjust the error message accordingly).
I personally prefer the later as arbitarily designating boundaries like this feels wrong. The user supposed knows their intend better than we do, so we shouldnt guess it.

Defaults to 0n or max - 999999999999999n if max is negative.

Vs

FakerError: Min not specified when using negative max option.

Or something similar.

If you just need a negative value you could just use -faker.number.bigInt()

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.97%. Comparing base (ece4f16) to head (5e4aa59).

Additional details and impacted files

@@            Coverage Diff             @@
##             next    #3363      +/-   ##
==========================================
- Coverage   99.97%   99.97%   -0.01%     
==========================================
  Files        2811     2811              
  Lines      217023   217028       +5     
  Branches      942      944       +2     
==========================================
+ Hits       216972   216975       +3     
- Misses         51       53       +2     

Files with missing lines	Coverage Δ
src/modules/number/index.ts	100.00% <100.00%> (ø)

... and 1 file with indirect coverage changes

[matthewmayer]

I'm not sure about it.

Afaict all our number methods return 0-max if only max is specified. So either we have to change all methods to behave like this or the user has to specify both min and max (and maybe we have to adjust the error message accordingly).

I personally prefer the later as arbitarily designating boundaries like this feels wrong. The user supposed knows their intend better than we do, so we shouldnt guess it.

Defaults to 0n or max - 999999999999999n if max is negative.

Vs

FakerError: Min not specified when using negative max option.

Or something similar.

If you just need a negative value you could just use -faker.number.bigInt()

I agree

I think it's fairly intuitive what faker.number.int({max:10}) returns

I don't think it's intuitive what

faker.number.int({max:-10}) returns

[ST-DDT]

@soc221b Could you please explain your thoughts regarding this change?
(Or the original issue you ran into?)

[ST-DDT]

I changed the target branch from v9 to next, as v9 is our release branch and all our development happens on the next branch.
(Unless we specifically back port changes to older versions)

[soc221b]

Thanks for your comments about this.

---

Could you please explain your thoughts regarding this change?

I personally think giving a negative max is a valid usage even it doesn't seem intuitive.

the original issue you ran into?

I'm building a library to generate fake data based on faker.js and ran into this issue: https://github.com/soc221b/zod-schema-faker/blob/1.1.0/src/zod-bigint-faker.ts#L26-L29

---

I created an example of form validation using <input max="-10">, it works as expected:
https://stackblitz.com/edit/does-negative-max-work?file=index.html

[ST-DDT]

I'm building a library to generate fake data based on faker.js and ran into this issue: https://github.com/soc221b/zod-schema-faker/blob/1.1.0/src/zod-bigint-faker.ts#L26-L29

I've looked at your code and noticed that you also have some code for multipleOf (similar to our faker.number.int({ multipleOf })).
Would you like to create a feature request issue for that as well?

[soc221b]

Would you like to create a feature request issue for that as well?

Sure, I'll create another PR for that. 😃

[ST-DDT]

Workaround for faker.number.bigInt({ max: -1n}):

-faker.number.bigInt({min: 1})

or with full context:

if (min == null && max != null && max < 0) {
  return -faker.number.bigInt({ min: -max });
}

[ST-DDT]

See also: #3364

• Inconsistent default of faker.number.bigInt max option  #3364