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: introduce faker.clone and derive #1499

Draft
wants to merge 1 commit into
base: next
Choose a base branch
from

Conversation

ST-DDT
Copy link
Member

@ST-DDT ST-DDT commented Oct 30, 2022

Fixes #627

Required for:

See also:


This PR introduces two new methods for faker.

  • faker.clone()
  • faker.derive()

clone()

Can be used to generate repeated values or to allow generating the "last" value again.
This method does not consume seed values.

Example

faker.seed(42);
faker.clone().datatype.number(); // 8234
faker.clone().datatype.number(); // 8234
faker.datatype.number(); // 8234

last() was originally requested in #627

derive()

Can be used to write methods that don't affect the rest of the generated data when changed.
This method consumes a single seed value in order to initialize the seed of the derived instance.

Example

function generatePerson(): Person {
  const derived = faker.derive();
  return {
    firstName: derived.person.firstName(),
    lastName: derived.person.lastName(),
    // You can add more attributes here without affecting the next person
    ...
  };
}

faker.seed(42);
generatePerson(); // { firstName: 'Devyn', lastName: 'Foo', ... }
generatePerson(); // { firstName: 'John', lastName: 'Bar', ... }

I evolved the fork() method into derive() while thinking about #1491 as it is incredible hard for our users to write extensible methods while retaining their original data. It basically makes generatePerson or any similar method an atomic operation regardless how many data are generated.

It can also be used to with nesting.

function generateMarriageCertificate(faker: Faker): MarriageCertificate {
  const derived = faker.derive();
  return {
    partner1: generatePerson(derived),
    partner2: generatePerson(derived),
    certifyingOffice: derived.location.city(),
  };
}

function generatePerson(faker: Faker): Person {
  const derived = faker.derive();
  return {
    firstName: derived.person.firstName(),
    lastName: derived.person.lastName(),
    // You can add more attributes here without affecting the next person
    ...
  };
}

Alternatives

  1. Add a context option to clone(), that mutates the seed (current seed + context -> new seed).
    However, I think the consumption of a seed value is actually expected, so that partner/marriage certificate 1 and 2 are different.

  2. Add a seed option to clone and use it like faker.fork({seed: faker.datatype.number()}).
    However that would just be a a more verbose version of the current derive method.


Food for thoughts: We could use the derive method in our code/methods to ensure each faker method call consumes only a single seed value. However this would probably (slightly) decrease performance.

@ST-DDT ST-DDT added c: feature Request for new feature p: 1-normal Nothing urgent labels Oct 30, 2022
@ST-DDT ST-DDT added this to the v8.0 - Module Re-Shuffling milestone Oct 30, 2022
@ST-DDT ST-DDT requested review from a team October 30, 2022 15:50
@ST-DDT ST-DDT self-assigned this Oct 30, 2022
@codecov
Copy link

codecov bot commented Oct 30, 2022

Codecov Report

Attention: 26 lines in your changes are missing coverage. Please review.

Comparison is base (bc3ebb7) 99.57% compared to head (93dd407) 99.56%.

Additional details and impacted files
@@            Coverage Diff             @@
##             next    #1499      +/-   ##
==========================================
- Coverage   99.57%   99.56%   -0.01%     
==========================================
  Files        2807     2807              
  Lines      250526   250652     +126     
  Branches     1153     1161       +8     
==========================================
+ Hits       249464   249574     +110     
- Misses       1034     1078      +44     
+ Partials       28        0      -28     
Files Coverage Δ
src/faker.ts 90.22% <100.00%> (+0.33%) ⬆️
src/internal/mersenne.ts 98.16% <100.00%> (+0.71%) ⬆️
src/simple-faker.ts 100.00% <100.00%> (ø)
src/randomizer.ts 0.00% <0.00%> (ø)

... and 28 files with indirect coverage changes

src/internal/mersenne/mersenne.ts Outdated Show resolved Hide resolved
src/internal/mersenne/twister.ts Outdated Show resolved Hide resolved
Shinigami92
Shinigami92 previously approved these changes Oct 31, 2022
src/faker.ts Outdated Show resolved Hide resolved
@xDivisionByZerox xDivisionByZerox requested a review from a team February 13, 2023 18:53
@xDivisionByZerox xDivisionByZerox added the s: accepted Accepted feature / Confirmed bug label Feb 13, 2023
@ST-DDT ST-DDT added the needs rebase There is a merge conflict label Mar 7, 2023
@ST-DDT ST-DDT removed the needs rebase There is a merge conflict label Mar 14, 2023
src/faker.ts Outdated Show resolved Hide resolved
@ST-DDT
Copy link
Member Author

ST-DDT commented Jul 20, 2023

Ready for review

Copy link
Member

@xDivisionByZerox xDivisionByZerox left a comment

Choose a reason for hiding this comment

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

Great documentation, @ST-DDT! Really enjoyed reading through it 😄

@ST-DDT ST-DDT removed the do NOT merge yet Do not merge this PR into the target branch yet label Jul 21, 2023
docs/guide/usage.md Outdated Show resolved Hide resolved
docs/guide/usage.md Outdated Show resolved Hide resolved
docs/guide/usage.md Outdated Show resolved Hide resolved
docs/guide/usage.md Outdated Show resolved Hide resolved
docs/guide/usage.md Outdated Show resolved Hide resolved
docs/guide/usage.md Show resolved Hide resolved
src/internal/mersenne/mersenne.ts Outdated Show resolved Hide resolved
src/internal/mersenne/twister.ts Outdated Show resolved Hide resolved
test/faker.spec.ts Outdated Show resolved Hide resolved
@ST-DDT ST-DDT requested a review from Shinigami92 July 29, 2023 12:04
@ST-DDT ST-DDT added the s: on hold Blocked by something or frozen to avoid conflicts label Jul 31, 2023
@ST-DDT ST-DDT added the s: needs decision Needs team/maintainer decision label Jul 31, 2023
@ST-DDT
Copy link
Member Author

ST-DDT commented Jul 31, 2023

@ST-DDT ST-DDT added needs rebase There is a merge conflict and removed s: on hold Blocked by something or frozen to avoid conflicts labels Oct 4, 2023
@ST-DDT ST-DDT modified the milestones: v8.x, v9.0 Oct 5, 2023
@ST-DDT ST-DDT added do NOT merge yet Do not merge this PR into the target branch yet s: needs decision Needs team/maintainer decision and removed needs rebase There is a merge conflict s: needs decision Needs team/maintainer decision labels Oct 5, 2023
@ST-DDT ST-DDT requested review from a team October 5, 2023 00:06
@Shinigami92
Copy link
Member

This would solve the problem of copycat: https://github.com/snaplet/copycat#the-problem

@ST-DDT
Copy link
Member Author

ST-DDT commented Oct 6, 2023

This would solve the problem of copycat: snaplet/copycat#the-problem

I would recommend using a custom Randomizer (!=Mersenne) e.g. from pure-random, that has shorter initialization times.
The Mersenne Twister is optimized to generate multiple random values.

This will also (potentially) get much more easier with v9's independent methods that can be called just like this firstName(fakerCore, ...args) that doesn't need a full faker instance anymore.

@ST-DDT ST-DDT force-pushed the feat/faker-fork-and-dervice branch from cb4992f to aefd4f0 Compare February 9, 2024 20:47
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 do NOT merge yet Do not merge this PR into the target branch yet p: 1-normal Nothing urgent s: accepted Accepted feature / Confirmed bug
Projects
No open projects
Status: Awaiting Review
4 participants