Skip to content

Conversation

soc221b
Copy link
Contributor

@soc221b soc221b commented Feb 16, 2025

Adds the multipleOf option to faker.number.bigInt():

@soc221b soc221b requested a review from a team as a code owner February 16, 2025 05:33
Copy link

netlify bot commented Feb 16, 2025

Deploy Preview for fakerjs ready!

Name Link
🔨 Latest commit bbabb7f
🔍 Latest deploy log https://app.netlify.com/sites/fakerjs/deploys/67c9d739a6bc1100080d4f26
😎 Deploy Preview https://deploy-preview-3402.fakerjs.dev
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

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

@ST-DDT ST-DDT added c: feature Request for new feature p: 1-normal Nothing urgent m: number Something is referring to the number module labels Feb 16, 2025
@ST-DDT ST-DDT added this to the vAnytime milestone Feb 16, 2025
@ST-DDT ST-DDT requested a review from a team February 16, 2025 08:57
Copy link

codecov bot commented Feb 16, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.97%. Comparing base (ae9aec6) to head (bbabb7f).
Report is 1 commits behind head on next.

Additional details and impacted files
@@           Coverage Diff           @@
##             next    #3402   +/-   ##
=======================================
  Coverage   99.97%   99.97%           
=======================================
  Files        2811     2811           
  Lines      217398   217408   +10     
  Branches      945      950    +5     
=======================================
+ Hits       217343   217355   +12     
+ Misses         55       53    -2     
Files with missing lines Coverage Δ
src/modules/number/index.ts 100.00% <100.00%> (ø)

... and 1 file with indirect coverage changes

🚀 New features to boost your workflow:
  • Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@ST-DDT
Copy link
Member

ST-DDT commented Feb 16, 2025

Thanks for your contribution. ❤️
I'll look at it more closely soon.
Have a nice weekend

@ST-DDT
Copy link
Member

ST-DDT commented Feb 18, 2025

I finally had some time to look into you PR and run some tests locally.
I found a few cases that behave differently from the existing methods:

faker.number.int({ 'min': 6, 'max': 9, 'multipleOf': 5 }) // Error: No suitable integer value between 6 and 9 found.
faker.number.bigInt({ 'min': 6, 'max': 9, 'multipleOf': 5 }) // 10
faker.number.int({ 'min': -9, 'max': 0, 'multipleOf': 5 }) // -5
faker.number.bigInt({ 'min': -9, 'max': 0, 'multipleOf': 5 }) // Error: Multiple of 5n should be less than or equal to max 0n.

Please fix these cases and add tests for them.

@soc221b soc221b marked this pull request as draft February 22, 2025 04:59
@soc221b soc221b marked this pull request as ready for review February 22, 2025 06:42
Copy link
Member

@ST-DDT ST-DDT left a comment

Choose a reason for hiding this comment

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

Sorry, for the slow review. It looks good to me, just a few suggestions to make it easier to read.

@soc221b
Copy link
Contributor Author

soc221b commented Mar 1, 2025

Sorry, for the slow review. It looks good to me, just a few suggestions to make it easier to read.

Thank you for your review. I have made the changes you suggested.

@ST-DDT ST-DDT requested a review from a team March 1, 2025 10:56
@ST-DDT
Copy link
Member

ST-DDT commented Mar 6, 2025

Thanks for your contribution ❤️

@ST-DDT ST-DDT enabled auto-merge (squash) March 6, 2025 17:12
@ST-DDT ST-DDT merged commit 7b4f85a into faker-js:next Mar 6, 2025
21 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c: feature Request for new feature m: number Something is referring to the number module p: 1-normal Nothing urgent
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants