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

refactor(mersenne): rewrite internal mersenne #1447

Merged
merged 15 commits into from
Oct 30, 2022
Merged

Conversation

Shinigami92
Copy link
Member

@Shinigami92 Shinigami92 commented Oct 14, 2022

Splitted from #1444

Changes:

  • This changes the Mersenne module to be not anymore a class but a light weight function
  • Also the code was rewritten so it is more easily maintainable
  • Additionally seed does not throw anymore a FakerError on invalid input -> this has also the benefit of that mersenne is now fully decoupled from faker itself and could theoretically be swapped with a generator, but this could be done in another PR

@Shinigami92 Shinigami92 added the c: chore PR that doesn't affect the runtime behavior label Oct 14, 2022
@Shinigami92 Shinigami92 self-assigned this Oct 14, 2022
@Shinigami92 Shinigami92 added p: 1-normal Nothing urgent do NOT merge yet Do not merge this PR into the target branch yet labels Oct 14, 2022
@codecov
Copy link

codecov bot commented Oct 14, 2022

Codecov Report

Merging #1447 (fd47e07) into next (4da3d5e) will decrease coverage by 0.00%.
The diff coverage is 100.00%.

❗ Current head fd47e07 differs from pull request most recent head b415bf6. Consider uploading reports for the commit b415bf6 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##             next    #1447      +/-   ##
==========================================
- Coverage   99.63%   99.63%   -0.01%     
==========================================
  Files        2175     2175              
  Lines      237647   237613      -34     
  Branches     1021     1009      -12     
==========================================
- Hits       236785   236751      -34     
  Misses        841      841              
  Partials       21       21              
Impacted Files Coverage Δ
src/faker.ts 95.04% <100.00%> (-0.05%) ⬇️
src/internal/mersenne/mersenne.ts 100.00% <100.00%> (ø)
src/modules/datatype/index.ts 99.12% <100.00%> (+<0.01%) ⬆️

@Shinigami92 Shinigami92 force-pushed the 1348-mark-mersenne-module-as-internal-and-hide-it-from-ts-and-the-api-docs branch from 56c7a95 to c97b0d2 Compare October 14, 2022 19:38
Base automatically changed from 1348-mark-mersenne-module-as-internal-and-hide-it-from-ts-and-the-api-docs to next October 14, 2022 23:49
@Shinigami92 Shinigami92 force-pushed the 1348-refactor-mersenne branch from d2e83bb to dd3689a Compare October 15, 2022 00:07
@Shinigami92 Shinigami92 removed the do NOT merge yet Do not merge this PR into the target branch yet label Oct 15, 2022
@Shinigami92 Shinigami92 requested a review from a team October 15, 2022 00:08
@Shinigami92 Shinigami92 marked this pull request as ready for review October 15, 2022 00:09
@Shinigami92 Shinigami92 requested a review from a team as a code owner October 15, 2022 00:09
@ST-DDT
Copy link
Member

ST-DDT commented Oct 15, 2022

IMO we could remove the mersenneFn wrapper and integrate these functions in the faker class.
We would then ask faker for the next seeded value instead of accessing faker's private mersenne property and doing things with it.

@xDivisionByZerox
Copy link
Member

IMO we could remove the mersenneFn wrapper and integrate these functions in the faker class. We would then ask faker for the next seeded value instead of accessing faker's private mersenne property and doing things with it.

I think that's the wrong way around. Currently, Faker is tightly coupled with the Mersenne generator. A way better approach would be to require a generator (probably denied from an interface) on construction, thus making the seed generation dependency injectable (follow the dependency inversion principle). This way I could build myself a Faker instance with the underlying generator I want.

interface SeedGenerator {
  next(): number;
}

class Faker {
  // properties
  constructor(options: {
    // current faker options
    generator: SeedGenerator; // could default to a mersenne implementation
  }) { 
    // initialization
  }
}

const instance = new Faker({
  ...defaultFakerOptions,
  generator: { next: () => 50 },
]);

In my eyes that's a much better approach since it allows for a higher level of abstraction and customization.

@Shinigami92 Shinigami92 changed the title chore: rewrite mersenne refactor(mersenne): rewrite internal mersenne Oct 16, 2022
@Shinigami92 Shinigami92 added c: refactor PR that affects the runtime behavior, but doesn't add new features or fixes bugs and removed c: chore PR that doesn't affect the runtime behavior labels Oct 16, 2022
@Shinigami92 Shinigami92 requested review from xDivisionByZerox, ST-DDT and a team October 16, 2022 09:55
ST-DDT
ST-DDT previously approved these changes Oct 16, 2022
src/internal/mersenne/mersenne.ts Outdated Show resolved Hide resolved
ST-DDT
ST-DDT previously approved these changes Oct 16, 2022
@ST-DDT ST-DDT added the s: accepted Accepted feature / Confirmed bug label Oct 16, 2022
ST-DDT
ST-DDT previously approved these changes Oct 16, 2022
@ST-DDT ST-DDT requested review from import-brain and a team October 29, 2022 10:43
@ST-DDT ST-DDT enabled auto-merge (squash) October 29, 2022 10:44
@ST-DDT ST-DDT merged commit 9abfcfb into next Oct 30, 2022
@ST-DDT ST-DDT deleted the 1348-refactor-mersenne branch October 30, 2022 08:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c: refactor PR that affects the runtime behavior, but doesn't add new features or fixes bugs p: 1-normal Nothing urgent s: accepted Accepted feature / Confirmed bug
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Mark Mersenne module as internal and hide it from ts and the api-docs
4 participants