Skip to content

fix: improve default seed initialization#1334

Merged
ST-DDT merged 2 commits intofaker-js:mainfrom
shraddhafalane:bug_fix
Sep 8, 2022
Merged

fix: improve default seed initialization#1334
ST-DDT merged 2 commits intofaker-js:mainfrom
shraddhafalane:bug_fix

Conversation

@shraddhafalane
Copy link
Copy Markdown
Contributor

@shraddhafalane shraddhafalane commented Sep 6, 2022 •
edited
Loading

closes #1333

@shraddhafalane shraddhafalane requested a review from a team as a code owner September 6, 2022 08:52
@ST-DDT ST-DDT added c: bug Something isn't working p: 2-high Fix main branch s: accepted Accepted feature / Confirmed bug labels Sep 6, 2022
@ST-DDT ST-DDT added this to the v7 - Current Major milestone Sep 6, 2022
ST-DDT
ST-DDT approved these changes Sep 6, 2022
View reviewed changes
Copy link
Copy Markdown
Member

@ST-DDT ST-DDT left a comment

Choose a reason for hiding this comment

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

Looks good to me.

Is there a way to test whether this fixes the original issue?

@codecov
Copy link
Copy Markdown

codecov Bot commented Sep 6, 2022 •
edited
Loading

Codecov Report

Merging #1334 (b9884d0) into main (b9884d0) will not change coverage.
The diff coverage is n/a.

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

@@           Coverage Diff           @@
##             main    #1334   +/-   ##
=======================================
  Coverage   99.62%   99.62%           
=======================================
  Files        2163     2163           
  Lines      241264   241264           
  Branches     1011     1011           
=======================================
  Hits       240354   240354           
  Misses        889      889           
  Partials       21       21

@Shinigami92
Copy link
Copy Markdown
Member

Shinigami92 commented Sep 7, 2022

Is there a way to test whether this fixes the original issue?

Maybe we could add a test using https://vitest.dev/guide/features.html#running-tests-concurrently 🤔
But not sure if this really works

@ST-DDT ST-DDT requested review from a team September 7, 2022 20:49
@ejcheng ejcheng added the needs test More tests are needed label Sep 7, 2022
ejcheng
ejcheng approved these changes Sep 7, 2022
View reviewed changes
Copy link
Copy Markdown
Member

@ejcheng ejcheng left a comment

Choose a reason for hiding this comment

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

Also LGTM, we definitely need to test this though

@ST-DDT ST-DDT changed the title fix: Improve default seed initialization fix: improve default seed initialization Sep 8, 2022
@ST-DDT ST-DDT enabled auto-merge (squash) September 8, 2022 17:24
@ST-DDT ST-DDT removed the needs test More tests are needed label Sep 8, 2022
@ST-DDT ST-DDT merged commit 925db3a into faker-js:main Sep 8, 2022
@xDivisionByZerox xDivisionByZerox mentioned this pull request Sep 10, 2022
Merged
@ST-DDT ST-DDT mentioned this pull request Nov 2, 2022
Closed
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 approved these changes

@ejcheng ejcheng ejcheng approved these changes

Assignees

@shraddhafalane shraddhafalane

Labels

c: bug Something isn't working p: 2-high Fix main branch s: accepted Accepted feature / Confirmed bug

Projects

No open projects
Faker Roadmap
Status: Done

Milestone

v7 - Current Major

Development

Successfully merging this pull request may close these issues.

Improve default seed initialization

4 participants

@shraddhafalane @Shinigami92 @ST-DDT @ejcheng