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

feat(helpers): add new faker.helpers.weightedArrayElement #1654

Merged
merged 36 commits into from
Jan 2, 2023

Conversation

matthewmayer
Copy link
Contributor

Split from #1637

Adds a new function faker.helpers.weightedArrayElement which allows you
to select a random element from an array, but weighted more to certain
elements than others. This will be useful for refactoring name patterns,
where for example you might want to have

  • FirstName Surname chosen 90% of the time and
  • FirstName Surname Suffix chosen 10% of the time

@matthewmayer matthewmayer requested a review from a team as a code owner December 11, 2022 03:36
@matthewmayer matthewmayer changed the title feat: add new faker.helpers.weightedArrayElement function feat (helpers): add new faker.helpers.weightedArrayElement function Dec 11, 2022
@matthewmayer matthewmayer changed the title feat (helpers): add new faker.helpers.weightedArrayElement function feat (helpers): add new faker.helpers.weightedArrayElement Dec 11, 2022
@codecov
Copy link

codecov bot commented Dec 11, 2022

Codecov Report

Merging #1654 (c0ba03b) into next (2ac5db9) will decrease coverage by 0.00%.
The diff coverage is 93.87%.

Additional details and impacted files
@@            Coverage Diff             @@
##             next    #1654      +/-   ##
==========================================
- Coverage   99.64%   99.64%   -0.01%     
==========================================
  Files        2245     2245              
  Lines      240269   240318      +49     
  Branches     1069     1078       +9     
==========================================
+ Hits       239407   239453      +46     
- Misses        841      844       +3     
  Partials       21       21              
Impacted Files Coverage Δ
src/modules/helpers/index.ts 98.86% <93.87%> (-0.30%) ⬇️

@matthewmayer matthewmayer changed the title feat (helpers): add new faker.helpers.weightedArrayElement feat(helpers): add new faker.helpers.weightedArrayElement Dec 11, 2022
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.

I think we should swap the weight and the value (weight first), because the weight is usually much shorter than the value making it easier to spot in longer lists. What do you/the others think?

[
  [ 5, 'sunny' ],
  [ 4, 'rainy' ],
  [ 1, 'extremely hazardous blizard storm clouds' ]
]
[
  [ 'sunny', 5 ],
  [ 'rainy', 4 ],
  [ 'extremely hazardous blizard storm clouds', 1 ]
]

src/modules/helpers/index.ts Outdated Show resolved Hide resolved
src/modules/helpers/index.ts Show resolved Hide resolved
src/modules/helpers/index.ts Outdated Show resolved Hide resolved
test/helpers.spec.ts Show resolved Hide resolved
src/modules/helpers/index.ts Outdated Show resolved Hide resolved
@ST-DDT
Copy link
Member

ST-DDT commented Dec 11, 2022

Should we allow weight zero for temporarily disabled or dynamically weighted items or should we expect the user to remove it before passing it to our method?

@Shinigami92 Shinigami92 added c: feature Request for new feature m: helpers Something is referring to the helpers module labels Dec 11, 2022
test/helpers.spec.ts Outdated Show resolved Hide resolved
test/helpers.spec.ts Outdated Show resolved Hide resolved
test/helpers.spec.ts Outdated Show resolved Hide resolved
test/helpers.spec.ts Show resolved Hide resolved
Matt Mayer and others added 3 commits December 11, 2022 19:47
@matthewmayer
Copy link
Contributor Author

Should we allow weight zero for temporarily disabled or dynamically weighted items or should we expect the user to remove it before passing it to our method?

i think allowing zeroes causes more confusion, for example what would you expect to happen if your array had three items, all with weight zero?

@Shinigami92
Copy link
Member

Should we allow weight zero for temporarily disabled or dynamically weighted items or should we expect the user to remove it before passing it to our method?

i think allowing zeroes causes more confusion, for example what would you expect to happen if your array had three items, all with weight zero?

All have same chance
so 1/3, 1/3 and 1/3

@matthewmayer
Copy link
Contributor Author

Should we allow weight zero for temporarily disabled or dynamically weighted items or should we expect the user to remove it before passing it to our method?

i think allowing zeroes causes more confusion, for example what would you expect to happen if your array had three items, all with weight zero?

All have same chance so 1/3, 1/3 and 1/3

Intuitively yes, but that would require some extra code to deal with that case, which i think adds more complexity and chance for bugs?

@matthewmayer
Copy link
Contributor Author

I note that faker.helpers.float defaults to a precision of 0.01, which means this is biased for small percentages. What's the cleanest way to request the max precision for the random float?

src/modules/helpers/index.ts Outdated Show resolved Hide resolved
src/modules/helpers/index.ts Outdated Show resolved Hide resolved
src/modules/helpers/index.ts Outdated Show resolved Hide resolved
@ST-DDT
Copy link
Member

ST-DDT commented Dec 11, 2022

I note that faker.helpers.float defaults to a precision of 0.01, which means this is biased for small percentages. What's the cleanest way to request the max precision for the random float?

Maybe open a new issue for that (max precision float)?

ST-DDT
ST-DDT previously approved these changes Dec 30, 2022
@ST-DDT ST-DDT requested review from Shinigami92 and a team December 31, 2022 11:01
Shinigami92
Shinigami92 previously approved these changes Dec 31, 2022
@Shinigami92 Shinigami92 enabled auto-merge (squash) December 31, 2022 17:52
@ST-DDT
Copy link
Member

ST-DDT commented Dec 31, 2022

The linter now complains about this.

auto-merge was automatically disabled January 1, 2023 03:45

Head branch was pushed to by a user without write access

@ST-DDT ST-DDT requested a review from a team January 1, 2023 21:27
@Shinigami92 Shinigami92 merged commit 59824e6 into faker-js:next Jan 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c: feature Request for new feature 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 this pull request may close these issues.

4 participants