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: support custom randomizer #2284

Merged
merged 33 commits into from Oct 4, 2023
Merged

feat: support custom randomizer #2284

merged 33 commits into from Oct 4, 2023

Conversation

ST-DDT
Copy link
Member

@ST-DDT ST-DDT commented Jul 31, 2023

Adds the ability to Faker to use a custom prng.

Intended use case for clone and derive:

  clone(): Faker {
    const instance = new Faker({
      locale: this.rawDefinitions,
      prng: this.prng.clone(),
    });
    instance.setDefaultRefDate(this._defaultRefDate);
    return instance;
  }

  derive(): Faker {
    const instance = new Faker({
      locale: this.rawDefinitions,
      prng: this.prng.derive(),
    });
    instance.setDefaultRefDate(this._defaultRefDate);
    return instance;
  }

(Regardless of whether we will actually have either of them)

Intended use case cross compatibility with other libraries other than faker:

const prng = newPRNG();
const customFaker = new Faker({ ..., prng });
const randExp = new new RandExp(/no{1,}/, { prng });

prng.setSeed(1337);
customFaker.number.int(); // 1st seed value
randExp.gen(); // 2nd seed value
customFaker.string.sample(); // 3rd seed value

Note: Currently faker's internal PRNG isn't exported.

@ST-DDT ST-DDT added c: feature Request for new feature p: 1-normal Nothing urgent labels Jul 31, 2023
@ST-DDT ST-DDT requested a review from a team as a code owner July 31, 2023 21:26
@ST-DDT ST-DDT self-assigned this Jul 31, 2023
@ST-DDT ST-DDT linked an issue Jul 31, 2023 that may be closed by this pull request
@codecov
Copy link

codecov bot commented Jul 31, 2023

Codecov Report

Merging #2284 (f98c2ee) into next (88b2443) will decrease coverage by 0.03%.
The diff coverage is 58.33%.

Additional details and impacted files
@@            Coverage Diff             @@
##             next    #2284      +/-   ##
==========================================
- Coverage   99.61%   99.58%   -0.03%     
==========================================
  Files        2820     2820              
  Lines      253880   253975      +95     
  Branches     1101     1102       +1     
==========================================
+ Hits       252891   252923      +32     
- Misses        962     1024      +62     
- Partials       27       28       +1     
Files Coverage Δ
src/faker.ts 89.13% <100.00%> (+0.90%) ⬆️
src/index.ts 100.00% <100.00%> (ø)
src/internal/mersenne.ts 96.29% <100.00%> (ø)
src/modules/number/index.ts 100.00% <100.00%> (ø)
src/simple-faker.ts 100.00% <100.00%> (ø)
src/randomizer.ts 0.00% <0.00%> (ø)

... and 2 files with indirect coverage changes

@ST-DDT ST-DDT requested a review from a team July 31, 2023 21:37
@ST-DDT ST-DDT added the s: needs decision Needs team/maintainer decision label Aug 6, 2023
@ST-DDT
Copy link
Member Author

ST-DDT commented Aug 6, 2023

We have to decide whether we want to merge this in v8.x or v9.
If we merge it in v9 we can rename the option/field/type to random/Random without breaking things/causing confusion.

@ST-DDT
Copy link
Member Author

ST-DDT commented Sep 7, 2023

Blocked by #2361 as it may require changes to the Mersenne interface.

@ST-DDT ST-DDT added the s: on hold Blocked by something or frozen to avoid conflicts label Sep 7, 2023
@Shinigami92
Copy link
Member

This PR should not be set on-hold, but the other way around: #2361 / #2357 should be set on hold and related to this PR
See the last two days of discussion in #2357

src/faker.ts Outdated Show resolved Hide resolved
@ST-DDT
Copy link
Member Author

ST-DDT commented Sep 15, 2023

This PR should not be set on-hold, but the other way around: #2361 / #2357 should be set on hold and related to this PR See the last two days of discussion in #2357

As mentioned in the team meeting. The exposed API is dependent on the decisions we make for those issues.
I don't like merging a public API PR, when it is likely to require changes or at least a more detailed specification.

@Shinigami92
Copy link
Member

This PR should not be set on-hold, but the other way around: #2361 / #2357 should be set on hold and related to this PR See the last two days of discussion in #2357

As mentioned in the team meeting. The exposed API is dependent on the decisions we make for those issues. I don't like merging a public API PR, when it is likely to require changes or at least a more detailed specification.

Ah ok I see, you want to wait half a week until we all decide and write down that we agree with the Math.random => [0.0,1.0) contract 👌

@Shinigami92 Shinigami92 changed the title feat: support custom prngs feat: support custom randomizer Sep 19, 2023
@Shinigami92 Shinigami92 removed s: on hold Blocked by something or frozen to avoid conflicts s: needs decision Needs team/maintainer decision labels Sep 19, 2023
src/randomizer.ts Outdated Show resolved Hide resolved
src/randomizer.ts Outdated Show resolved Hide resolved
src/randomizer.ts Show resolved Hide resolved
@matthewmayer
Copy link
Contributor

Should we add an example of how you go from a prebuilt localised Faker instance (say fakerEN_GB) to a version with a custom randomizer? That may not be that intuitive as I think you'd have to drop the prebuilt instance and use manual fallback locales in order to access the Faker constructor.

@ST-DDT
Copy link
Member Author

ST-DDT commented Sep 30, 2023

Could you please write it in pseudocode? I'm not sure what you are asking for.

@matthewmayer
Copy link
Contributor

matthewmayer commented Sep 30, 2023

// if you are currently using a prebuilt version of Faker 
import {fakerEN_GB as faker} from '@faker-js/faker';
console.log(faker.location.city())

// you will need to manually construct a new Faker instance with the correct fallback languages
// and then you can set a custom randomizer function
import { Faker, en, en_GB, base } from '@faker-js/faker';
const faker2 = new Faker({
  locale: [en_GB, en, base],
  randomizer: yourCustomRandomizerFunction
})
console.log(faker2.location.city())

@ST-DDT
Copy link
Member Author

ST-DDT commented Sep 30, 2023

I think adding the Faker constructor with some ommissions should be plenty. Maybe we should add a link to the constructor though.

Const faker2 = new Faker({
  locale: ...,
  randomizer: newRandomizer(),
});

And @see Faker#constructor

@matthewmayer
Copy link
Contributor

It's more guiding people that they cannot simply do something like

fakerEN_GB.randomizer = ...

@ST-DDT
Copy link
Member Author

ST-DDT commented Oct 2, 2023

Here the updated screenshots for the documentation:

Click to expand

http://localhost:5173/api/randomizer.html

grafik
grafik
grafik

http://localhost:5173/guide/randomizer.html

grafik
grafik
grafik
grafik
grafik

@dubzzz
Copy link

dubzzz commented Oct 3, 2023

From the updated documentation you sent, the API looks great. I'm looking forward to connect it 🥰

Thanks a lot for it 👍

@xDivisionByZerox xDivisionByZerox merged commit 5410239 into next Oct 4, 2023
20 checks passed
@xDivisionByZerox xDivisionByZerox deleted the feat/custom-prng branch October 4, 2023 16:13
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 p: 1-normal Nothing urgent
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Customize the PRNG used by faker
6 participants