Skip to content

feat: support custom randomizer#2284

Merged
xDivisionByZerox merged 33 commits into
nextfrom
feat/custom-prng
Oct 4, 2023
Merged

feat: support custom randomizer#2284
xDivisionByZerox merged 33 commits into
nextfrom
feat/custom-prng

Conversation

@ST-DDT

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

Copy link
Copy Markdown
Member

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

codecov Bot commented Jul 31, 2023

Copy link
Copy Markdown

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
Comment thread src/internal/mersenne.ts Outdated
@ST-DDT ST-DDT added the s: needs decision Needs team/maintainer decision label Aug 6, 2023
@ST-DDT

ST-DDT commented Aug 6, 2023

Copy link
Copy Markdown
Member Author

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

ST-DDT commented Sep 7, 2023

Copy link
Copy Markdown
Member Author

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
Copy Markdown
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

Comment thread src/faker.ts Outdated
@ST-DDT

ST-DDT commented Sep 15, 2023

Copy link
Copy Markdown
Member Author

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
Copy Markdown
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
@matthewmayer

Copy link
Copy Markdown
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

ST-DDT commented Sep 30, 2023

Copy link
Copy Markdown
Member Author

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

@matthewmayer

matthewmayer commented Sep 30, 2023

Copy link
Copy Markdown
Contributor
// 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

ST-DDT commented Sep 30, 2023

Copy link
Copy Markdown
Member Author

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
Copy Markdown
Contributor

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

fakerEN_GB.randomizer = ...

@ST-DDT

ST-DDT commented Oct 2, 2023

Copy link
Copy Markdown
Member Author

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

dubzzz commented Oct 3, 2023

Copy link
Copy Markdown

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
@xDivisionByZerox xDivisionByZerox deleted the feat/custom-prng branch October 4, 2023 16:13
@dubzzz

dubzzz commented Jul 18, 2024

Copy link
Copy Markdown

I want to thank you all for this awesome feature.

It unlocks a better integration of Faker within fast-check as detailed in this article.

If you read it and have some remarks, please let me know, I'll be happy to discuss and fix them.

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

6 participants