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

fix: move utils to the correct place #2964

Draft
wants to merge 5 commits into
base: next
Choose a base branch
from

Conversation

watercubz
Copy link

@watercubz watercubz commented Jun 22, 2024

Issue #2961: Move Functions and Types to Proper Directories

Changes Made

  • Moved Functions: Moved generateMersenne*Randomizers() functions to the src/utils directory.
  • File Moved: Moved src/utils/types.ts to the src/internal directory.

Additional Details

  • Updated imports and references throughout the project to reflect the new locations.
  • These changes ensure better organization of internal and external utilities as discussed in issue Move utils to the correct place #2961.

Screenshots

Attached screenshots showing the new directory structure and updated imports.
image
image
image
image

Related Issue

@watercubz watercubz requested a review from a team as a code owner June 22, 2024 02:34
Copy link

netlify bot commented Jun 22, 2024

Deploy Preview for fakerjs ready!

Built without sensitive environment variables

Name Link
🔨 Latest commit 7d10d0f
🔍 Latest deploy log https://app.netlify.com/sites/fakerjs/deploys/6681790bd1f6b00008f6185c
😎 Deploy Preview https://deploy-preview-2964.fakerjs.dev
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@watercubz watercubz changed the title Issue 2961 reorganize utils Move utils to the correct place Jun 22, 2024
@watercubz watercubz changed the title Move utils to the correct place fix: move utils to the correct place Jun 22, 2024
@watercubz watercubz changed the title fix: move utils to the correct place fix: move utils to the correct place #2964 Jun 22, 2024
@import-brain import-brain changed the title fix: move utils to the correct place #2964 fix: move utils to the correct place Jun 22, 2024
Copy link

codecov bot commented Jun 22, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.95%. Comparing base (92a2f17) to head (7baf89a).

Current head 7baf89a differs from pull request most recent head 7d10d0f

Please upload reports for the commit 7d10d0f to get more accurate results.

Additional details and impacted files
@@            Coverage Diff             @@
##             next    #2964      +/-   ##
==========================================
- Coverage   99.95%   99.95%   -0.01%     
==========================================
  Files        2984     2984              
  Lines      216057   216057              
  Branches      597      595       -2     
==========================================
- Hits       215969   215957      -12     
- Misses         88      100      +12     
Files Coverage Δ
src/faker.ts 100.00% <100.00%> (ø)
src/index.ts 100.00% <100.00%> (ø)
src/internal/mersenne.ts 97.58% <100.00%> (ø)
src/modules/string/index.ts 100.00% <100.00%> (ø)
src/simple-faker.ts 100.00% <100.00%> (ø)

... and 1 file with indirect coverage changes

@ST-DDT ST-DDT added this to the vAnytime milestone Jun 22, 2024
@ST-DDT ST-DDT linked an issue Jun 22, 2024 that may be closed by this pull request
@ST-DDT ST-DDT added c: chore PR that doesn't affect the runtime behavior p: 1-normal Nothing urgent labels Jun 22, 2024
@ST-DDT
Copy link
Member

ST-DDT commented Jun 22, 2024

@watercubz Please also add a comment to the issue so that we can assign it to you for better visibility.

Copy link
Member

Choose a reason for hiding this comment

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

Please also move the

export function generateMersenne32Randomizer(): Randomizer {

And the 53bit variant to src/utils/mersenne.ts

src/utils/randomizer.ts Outdated Show resolved Hide resolved
@Shinigami92
Copy link
Member

I think src/internal/module-base.ts should also moved to src/utils/module-base.ts if not even to src/modules/module-base.ts 🤔

@ST-DDT ST-DDT added the s: needs decision Needs team/maintainer decision label Jun 24, 2024
@ST-DDT
Copy link
Member

ST-DDT commented Jun 27, 2024

Team Decision

  • The randomizer will get their own folder because there will be multiple adapter implementations in the future. Since they are publically available they should NOT be located in the internal directory.
    • src/randomizer.ts -> src/randomizer/types.ts
    • src/internal/mersenne.ts {generateMersenne32Randomizer, generateMersenne53Randomizer} -> src/randomizer/mersenne.ts
  • These types are not exported and thus should be located in src/internal
    • src/utils/types.ts -> src/internal/types.ts
    • src/internal/module-base.ts will not be moved

@ST-DDT ST-DDT removed the s: needs decision Needs team/maintainer decision label Jun 27, 2024
Copy link
Member

Choose a reason for hiding this comment

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

issue (blocking): MersenneTwister19937 is an internal class and should stay in internal

Copy link
Member

Choose a reason for hiding this comment

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

Clarification: the mersenne class is internal, the generateXRandomizer utility functions are public.

Copy link
Author

Choose a reason for hiding this comment

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

Well, then mersenne goes internal, and randomizer would go outside of utils and types in internal along with mersenne, would that be the correction to the problem?

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Author

Choose a reason for hiding this comment

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

Of course I think I already understood the point, thank you very much, I will upload changes to those previously mentioned in a moment.

@ST-DDT ST-DDT marked this pull request as draft July 3, 2024 20:28
@Shinigami92
Copy link
Member

@watercubz is this ready for review again? You mentioned in #2964 (comment) that you want to upload something, but I cannot see further commits from there. Maybe you missed something?

If it is ready for review again, please click the "Ready for review" button so we get notified.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c: chore PR that doesn't affect the runtime behavior p: 1-normal Nothing urgent
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Move utils to the correct place
4 participants