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.

Sign up for GitHub

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

Jump to bottom

fix: move utils to the correct place #2964

Open
wants to merge 3 commits into
base: next
Choose a base branch
from
Open

fix: move utils to the correct place #2964

wants to merge 3 commits into from

Conversation

watercubz
Copy link

@watercubz watercubz commented Jun 22, 2024 •
edited
Loading

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
@netlify Netlify
Copy link

netlify bot commented Jun 22, 2024 •
edited
Loading

✅ Deploy Preview for fakerjs ready!

Name Link
🔨 Latest commit 7baf89a
🔍 Latest deploy log https://app.netlify.com/sites/fakerjs/deploys/667d98e653902b0008bbe8d9
😎 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
@codecov Codecov
Copy link

codecov bot commented Jun 22, 2024 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.95%. Comparing base (49d7119) to head (7baf89a).

Additional details and impacted files 
@@            Coverage Diff             @@
##             next    #2964      +/-   ##
==========================================
- Coverage   99.96%   99.95%   -0.02%     
==========================================
  Files        2984     2984              
  Lines      216057   216057              
  Branches      601      595       -6     
==========================================
- Hits       215984   215957      -27     
- Misses         73      100      +27
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 2 files 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
Move utils to the correct place #2961
Open
@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.

ST-DDT
ST-DDT reviewed Jun 22, 2024
View reviewed changes
src/internal/mersenne.ts
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

faker/src/internal/mersenne.ts

Line 333 in 05237e4

export function generateMersenne32Randomizer(): Randomizer {

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

src/utils/randomizer.ts
Copy link
Member

Choose a reason for hiding this comment

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

Not sure about this one. @ others: What do you think?

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, if it doesn't convince you, let me change it and add the other requests you told me.

Copy link
Member

@xDivisionByZerox xDivisionByZerox Jun 22, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Not sure about this one. @ others: What do you think?

From the issue:

External utilities [(src/utils/*.ts)] are not required to use a faker seed.

I believe we need to clarify our definition of "utilities." Is a type/interface always considered a utility, or only if it is generic? Is the Randomizer even a utility in the first place or a core building block of our library?

Additionally, I'd like to discuss the possibility of creating a separate directory, src/util/randomizer. This directory could house the randomizer interface and specific randomizer adapters (e.g., mersenne, pure-rand, ...). If this question isn't directly related to the current topic, feel free to disregard it.

Copy link
Member

Choose a reason for hiding this comment

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

IMO the interface for Randomizer should be exposed, because it is the return type of generate*Randomizer.
So users can write const randomizer: Randomizer = generateMersenne32Randomizer();

@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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ST-DDT ST-DDT ST-DDT left review comments

@Shinigami92 Shinigami92 Shinigami92 left review comments

@xDivisionByZerox xDivisionByZerox xDivisionByZerox left review comments

At least 1 approving review is required to merge this pull request.

Assignees

@watercubz watercubz

Labels
c: chore PR that doesn't affect the runtime behavior p: 1-normal Nothing urgent
Projects
None yet
Milestone
vAnytime
Development

Successfully merging this pull request may close these issues.

Move utils to the correct place
4 participants
@watercubz @ST-DDT @Shinigami92 @xDivisionByZerox