Skip to content

Support custom Random Generators#1562

Merged
kingthorin merged 1 commit intodatafaker-net:mainfrom
zyclonite:f/customRandom
Jun 1, 2025
Merged

Support custom Random Generators#1562
kingthorin merged 1 commit intodatafaker-net:mainfrom
zyclonite:f/customRandom

Conversation

@zyclonite
Copy link
Copy Markdown
Contributor

@zyclonite zyclonite commented May 30, 2025

Add support for using custom RandomGenerators like SplittableRandom for improved speed and less overhead

@what-the-diff
Copy link
Copy Markdown

what-the-diff bot commented May 30, 2025

PR Summary

  • Updated Locality.java

    • This PR incorporates a more modern and effective random number generator technique by transforming the random parameter's type from Random to RandomGenerator in localeStringWithRandom and localeStringWithoutReplacement methods.
    • It also adjusts the Collections.shuffle method to effectively work with the new RandomGenerator type.

  • Modified FakeValuesService.java

    • The code now imports the java.util.Random for potential future operational needs.
    • Minor formatting improvements have been introduced for consistency, including spaces around operators.

  • Altered RandomService.java

    • Changes have been made in the random field and the constructor element from Random to RandomGenerator.
    • The getRandomInternal method is now updated to yield RandomGenerator instead of Random.

  • Expanded RandomServiceTest.java

    • A new test method customRandomGenerator has been implemented which validates functionality when used with a SplittableRandom instance.

kingthorin
kingthorin reviewed May 30, 2025
View reviewed changes
Copy link
Copy Markdown
Collaborator

@kingthorin kingthorin left a comment

Choose a reason for hiding this comment

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

Could you revert the formatting changes?

Not that any of them are wrong but just to keep this focused on the relevant changes.

@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented May 30, 2025 •
edited
Loading

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 92.50%. Comparing base (212f8ae) to head (62f0c6e).
Report is 1 commits behind head on main.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files 
@@             Coverage Diff              @@
##               main    #1562      +/-   ##
============================================
+ Coverage     92.45%   92.50%   +0.04%     
- Complexity     3308     3312       +4     
============================================
  Files           329      329              
  Lines          6507     6510       +3     
  Branches        632      632              
============================================
+ Hits           6016     6022       +6     
+ Misses          338      337       -1     
+ Partials        153      151       -2

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 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.

@zyclonite zyclonite force-pushed the f/customRandom branch 2 times, most recently from 8a08712 to 512838b Compare May 30, 2025 18:00
@zyclonite
Copy link
Copy Markdown
Contributor Author

zyclonite commented May 30, 2025

Could you revert the formatting changes?

Not that any of them are wrong but just to keep this focused on the relevant changes.

this was by mistake, removed it now

kingthorin
kingthorin approved these changes May 30, 2025
View reviewed changes
Copy link
Copy Markdown
Collaborator

@kingthorin kingthorin left a comment

Choose a reason for hiding this comment

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

LGTM

asolntsev
asolntsev requested changes Jun 1, 2025
View reviewed changes
src/main/java/net/datafaker/providers/base/Locality.java Outdated
shuffledLocales.addAll(LOCALES);
shuffledLocaleIndex = 0;
Collections.shuffle(shuffledLocales, random);
Collections.shuffle(shuffledLocales, (Random) random);
Copy link
Copy Markdown
Collaborator

@asolntsev asolntsev Jun 1, 2025

Choose a reason for hiding this comment

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

Maybe I missed something, but what is the point of declaring parameter as RandomGenerator if we anyway cast it to Random? Then no any other types would work here...

Copy link
Copy Markdown
Contributor Author

@zyclonite zyclonite Jun 1, 2025

Choose a reason for hiding this comment

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

because this new method does start to exist in jdk21... this would break the compatibility for jdk17

Copy link
Copy Markdown
Contributor Author

@zyclonite zyclonite Jun 1, 2025

Choose a reason for hiding this comment

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

but good point - seems there is not test for that...

Copy link
Copy Markdown
Contributor Author

@zyclonite zyclonite Jun 1, 2025

Choose a reason for hiding this comment

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

added a way to fix it now... but the same is true for RgxGen, so this would need to be fixed first

kingthorin
kingthorin reviewed Jun 1, 2025
View reviewed changes
@zyclonite
Copy link
Copy Markdown
Contributor Author

zyclonite commented Jun 1, 2025

depends on curious-odd-man/RgxGen#108

@zyclonite
Copy link
Copy Markdown
Contributor Author

zyclonite commented Jun 1, 2025

the new version 3.0 of RgxGen supports RandomGenerator now
upgraded the version and fixed the implementation

@zyclonite zyclonite requested a review from asolntsev June 1, 2025 19:27
@asolntsev asolntsev added this to the 2.4.4 milestone Jun 1, 2025
@asolntsev asolntsev added the enhancement New feature or request label Jun 1, 2025
@kingthorin kingthorin merged commit 3af0054 into datafaker-net:main Jun 1, 2025
13 checks passed
@zyclonite zyclonite deleted the f/customRandom branch June 1, 2025 19:46
@kingthorin kingthorin mentioned this pull request Jun 20, 2025
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@kingthorin kingthorin kingthorin approved these changes

@asolntsev asolntsev asolntsev approved these changes

Assignees

No one assigned

Labels

enhancement New feature or request

Projects

None yet

Milestone

2.4.4

Development

Successfully merging this pull request may close these issues.

4 participants

@zyclonite @codecov-commenter @asolntsev @kingthorin