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

Remove default value from helpers.arrayElement() #581

Closed
ST-DDT opened this issue Feb 28, 2022 · 5 comments
Closed

Remove default value from helpers.arrayElement() #581

ST-DDT opened this issue Feb 28, 2022 · 5 comments
Assignees
Labels
c: refactor PR that affects the runtime behavior, but doesn't add new features or fixes bugs m: helpers Something is referring to the helpers module p: 1-normal Nothing urgent s: accepted Accepted feature / Confirmed bug
Milestone

Comments

@ST-DDT
Copy link
Member

ST-DDT commented Feb 28, 2022

faker/src/random.ts

Lines 106 to 108 in 4eef328

arrayElement<T = string>(
array: ReadonlyArray<T> = ['a', 'b', 'c'] as unknown as ReadonlyArray<T>
): T {

This causes issues with missing data, that then gets magically replaced with these defaults, which results in unexpected behavior.
This change could be considered breaking, but IMO calling that method without an array to choose from is a bug itself.
Thus why I consider this a bug, that needs fixing more than retaining backwards compatibility.

We observed this method misbehaving in our tests before.

Alternatives considered

Create a new method and deprecate the old one.

Additional Information

Related to #219

@ST-DDT ST-DDT added c: bug Something isn't working c: feature Request for new feature p: 3-urgent Fix and release ASAP labels Feb 28, 2022
@ST-DDT ST-DDT added this to the v6.1 - First bugfixes milestone Feb 28, 2022
@ST-DDT ST-DDT moved this to Todo in Faker Roadmap Feb 28, 2022
@xDivisionByZerox
Copy link
Member

xDivisionByZerox commented Mar 1, 2022

If calling the method with an empty array is considered a bug, what would be the correct behavior for empty arrays?

@Shinigami92
Copy link
Member

IMO we should "just" remove the = ['a', 'b', 'c'] as unknown as ReadonlyArray<T> part, so that a given argument is required.
But is it really a bug if calling it with an empty array? Cause this would be already a bug now 👀
I would assume that it would throw an error or just return undefined 🤔

@xDivisionByZerox
Copy link
Member

If it could return undefined you must extend the method signature with undefined as return value. This can be very exhausting for end-users (because undefined check). So I would say throwing an error would be the way to go.

@xDivisionByZerox
Copy link
Member

Can you assign me either way?

@Shinigami92 Shinigami92 added p: 1-normal Nothing urgent c: refactor PR that affects the runtime behavior, but doesn't add new features or fixes bugs and removed c: bug Something isn't working c: feature Request for new feature p: 3-urgent Fix and release ASAP labels Mar 15, 2022
@Shinigami92 Shinigami92 moved this from Todo to In Progress in Faker Roadmap Mar 15, 2022
@Shinigami92 Shinigami92 added the s: accepted Accepted feature / Confirmed bug label Mar 23, 2022
@xDivisionByZerox xDivisionByZerox changed the title Remove default value from random.arrayElement() Remove default value from helpers.arrayElement() Jul 30, 2022
@xDivisionByZerox xDivisionByZerox added the m: helpers Something is referring to the helpers module label Jul 30, 2022
@xDivisionByZerox
Copy link
Member

Fixed by #2045.

@github-project-automation github-project-automation bot moved this from In Progress to Done in Faker Roadmap Apr 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c: refactor PR that affects the runtime behavior, but doesn't add new features or fixes bugs m: helpers Something is referring to the helpers module p: 1-normal Nothing urgent s: accepted Accepted feature / Confirmed bug
Projects
No open projects
Status: Done
Development

Successfully merging a pull request may close this issue.

3 participants