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

Merge faker.defaultRefDate with faker.seed #1895

Closed
ST-DDT opened this issue Mar 5, 2023 · 3 comments
Closed

Merge faker.defaultRefDate with faker.seed #1895

ST-DDT opened this issue Mar 5, 2023 · 3 comments
Assignees
Labels
c: docs Improvements or additions to documentation good first issue Good for newcomers p: 1-normal Nothing urgent s: accepted Accepted feature / Confirmed bug
Milestone

Comments

@ST-DDT
Copy link
Member

ST-DDT commented Mar 5, 2023

Clear and concise description of the problem

When setting faker.seed it isn't clear that you also have to set a fixed defaultRefDate to get reproducible results for the faker.date.*() methods.

Suggested solution

Add a defaultRefDate parameter/option to faker.seed.

Worth discussing:

Should the current (v8 new) faker.defaultRefDate method be removed?

Alternative

Add @see to both methods.

Additional context

Originally suggested here #1870 (comment) by @luisjakon

@ST-DDT ST-DDT added p: 1-normal Nothing urgent s: needs decision Needs team/maintainer decision c: refactor PR that affects the runtime behavior, but doesn't add new features or fixes bugs labels Mar 5, 2023
@luisjakon
Copy link

luisjakon commented Mar 6, 2023

If it's too much of a polluting change, perhaps it might be easier to just have a note or a link on reproducibility and fixation of seed and dates upfront (or early on) in the description as previously suggested. This way, hopefully it's not as surprising as it was to me while testing and running the code. The other thing to consider is whether you might want to have an options object to seed or an engine init function that sets all useful parameters that faker may make use of in all other functions. Other than that, the current solution works fine and can be looked into in another revision. Thanks!

@Shinigami92
Copy link
Member

Team Decision

For now we will just aim for better docs (using the alternative @see)

@Shinigami92 Shinigami92 removed the s: needs decision Needs team/maintainer decision label Mar 9, 2023
@ST-DDT ST-DDT added good first issue Good for newcomers s: accepted Accepted feature / Confirmed bug c: docs Improvements or additions to documentation and removed c: refactor PR that affects the runtime behavior, but doesn't add new features or fixes bugs labels Mar 9, 2023
@ST-DDT ST-DDT self-assigned this Mar 29, 2023
@ST-DDT
Copy link
Member Author

ST-DDT commented Mar 29, 2023

Fixed via #1960

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c: docs Improvements or additions to documentation good first issue Good for newcomers p: 1-normal Nothing urgent s: accepted Accepted feature / Confirmed bug
Projects
No open projects
Status: Done
Development

No branches or pull requests

3 participants