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: faker.helpers.maybe #874

Merged
merged 7 commits into from
Apr 26, 2022
Merged

feat: faker.helpers.maybe #874

merged 7 commits into from
Apr 26, 2022

Conversation

Shinigami92
Copy link
Member

closes #870

GitHub Copilot generated some JSDocs and provided the naming maybe instead of optional, and I like that more, so I took that as name

@Shinigami92 Shinigami92 added c: feature Request for new feature p: 1-normal Nothing urgent s: accepted Accepted feature / Confirmed bug labels Apr 26, 2022
@Shinigami92 Shinigami92 added this to the v6.3 - Next Minor milestone Apr 26, 2022
@Shinigami92 Shinigami92 self-assigned this Apr 26, 2022
@Shinigami92 Shinigami92 added the needs test More tests are needed label Apr 26, 2022
src/random.ts Outdated Show resolved Hide resolved
src/random.ts Outdated Show resolved Hide resolved
Shinigami92 and others added 2 commits April 26, 2022 10:41
Co-authored-by: ST-DDT <ST-DDT@gmx.de>
@Shinigami92 Shinigami92 removed the needs test More tests are needed label Apr 26, 2022
@codecov
Copy link

codecov bot commented Apr 26, 2022

Codecov Report

Merging #874 (cdb08e0) into main (b41c662) will increase coverage by 0.00%.
The diff coverage is 100.00%.

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

@@           Coverage Diff           @@
##             main     #874   +/-   ##
=======================================
  Coverage   99.42%   99.42%           
=======================================
  Files        1958     1958           
  Lines      210862   210886   +24     
  Branches      919      922    +3     
=======================================
+ Hits       209649   209673   +24     
  Misses       1156     1156           
  Partials       57       57           
Impacted Files Coverage Δ
src/helpers.ts 99.30% <100.00%> (+0.02%) ⬆️

@Shinigami92 Shinigami92 marked this pull request as ready for review April 26, 2022 16:19
@Shinigami92 Shinigami92 requested a review from a team as a code owner April 26, 2022 16:19
@Shinigami92 Shinigami92 requested review from ST-DDT and a team April 26, 2022 16:20
ST-DDT
ST-DDT previously approved these changes Apr 26, 2022
@ST-DDT ST-DDT requested a review from a team April 26, 2022 18:34
@xDivisionByZerox
Copy link
Member

@ST-DDT proposed here to place this method in the helpers module. I think that would be better than exposing it in the random module.

@Shinigami92
Copy link
Member Author

@ST-DDT proposed here to place this method in the helpers module. I think that would be better than exposing it in the random module.

Oh, I oversaw this. But my current understatement is that random should handle values that are based on deterministic and helpers contains only stuff that are like utilities 🤔
Like slugify or repeatString
I also wrote that down here: #805 (reply in thread)

But I can be wrong and if you are voting for handling it differently, let's define that now and write that down somewhere.

@ST-DDT
Copy link
Member

ST-DDT commented Apr 26, 2022

But my current understatement is that random should handle values that are based on deterministic

For the return value that is. Here the value/source is passed by the user.

and helpers contains only stuff that are like utilities 🤔 Like slugify or repeatString

IMO this maybe is exactly a helper, as it simplifies certain tasks for the user, but the output is heavily dependent on the users input and not some locale.

I also wrote that down here: #805 (reply in thread)

I couldn't find the exact part that you were referring to, probably not:

Random

  • It could be fully removed? ...

  • For Helpers vote this post with the rocket 🚀 .
  • For Random vote this post with the party 🎉 .

@Shinigami92
Copy link
Member Author

Shinigami92 commented Apr 26, 2022

I was talking about

  • Helpers
    • Should only contain stuff that is not based on locale definitions!

For deterministic, I talked about

if (this.faker.datatype.float({ min: 0, max: 1 }) < probability) {

IMO it is like arrayElement, we should really come up with some well defined structure of modules so that questions like this cannot raise anymore in future...

@ST-DDT
Copy link
Member

ST-DDT commented Apr 26, 2022

IMO:

  • Helpers is for everything that transforms the input
  • Random generates new stuff

@Shinigami92
Copy link
Member Author

IMO:

  • Helpers is for everything that transforms the input
  • Random generates new stuff

I accept this, let's write it down. For now please update our internal Google Docs with that

@Shinigami92 Shinigami92 changed the title feat: faker.random.maybe feat: faker.helpers.maybe Apr 26, 2022
@Shinigami92 Shinigami92 requested a review from ST-DDT April 26, 2022 20:13
src/helpers.ts Outdated Show resolved Hide resolved
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.

grafik

@ST-DDT ST-DDT requested review from a team April 26, 2022 20:30
@ST-DDT ST-DDT requested a review from a team April 26, 2022 20:44
@Shinigami92 Shinigami92 enabled auto-merge (squash) April 26, 2022 20:47
@Shinigami92 Shinigami92 merged commit a64cbde into main Apr 26, 2022
@ST-DDT ST-DDT deleted the random-maybe branch April 29, 2022 20:00
@xDivisionByZerox xDivisionByZerox added the m: helpers Something is referring to the helpers module label Aug 26, 2022
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
None yet
Development

Successfully merging this pull request may close these issues.

feat: faker optional with probability
3 participants