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: improve default seed initialization #1334

Merged
merged 2 commits into from
Sep 8, 2022
Merged

fix: improve default seed initialization #1334

merged 2 commits into from
Sep 8, 2022

Conversation

shraddhafalane
Copy link
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
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

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
Member

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
@import-brain import-brain added the needs test More tests are needed label Sep 7, 2022
import-brain
import-brain approved these changes Sep 7, 2022
View reviewed changes
Copy link
Member

@import-brain import-brain 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

@import-brain import-brain import-brain approved these changes

@ST-DDT ST-DDT ST-DDT 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 @import-brain