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(datatype): introduce probability option to boolean #1476

Merged
merged 7 commits into from
Nov 3, 2022

Conversation

ST-DDT
Copy link
Member

@ST-DDT ST-DDT commented Oct 22, 2022

Throughout our code base the have some locations that use probabilities to decide what to do.
This PR merges these occurrences into our datatype.boolean() method and thus provides this option to others.

Docs Preview

grafik

@ST-DDT ST-DDT added c: feature Request for new feature p: 1-normal Nothing urgent s: accepted Accepted feature / Confirmed bug m: datatype Something is referring to the datatype module labels Oct 22, 2022
@ST-DDT ST-DDT requested review from a team October 22, 2022 12:07
@ST-DDT ST-DDT self-assigned this Oct 22, 2022
@codecov
Copy link

codecov bot commented Oct 22, 2022

Codecov Report

Merging #1476 (4825329) into next (4204a6a) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Additional details and impacted files
@@           Coverage Diff           @@
##             next    #1476   +/-   ##
=======================================
  Coverage   99.63%   99.64%           
=======================================
  Files        2175     2175           
  Lines      237613   237636   +23     
  Branches     1009     1013    +4     
=======================================
+ Hits       236751   236782   +31     
+ Misses        841      833    -8     
  Partials       21       21           
Impacted Files Coverage Δ
src/modules/datatype/index.ts 99.18% <100.00%> (+0.05%) ⬆️
src/modules/finance/index.ts 100.00% <100.00%> (ø)
src/modules/helpers/index.ts 98.44% <100.00%> (-0.01%) ⬇️
src/modules/internet/user-agent.ts 92.59% <0.00%> (+2.11%) ⬆️

Copy link
Member

@Shinigami92 Shinigami92 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking into the test snapshots, it looks like the behavior exactly swapped, true is now false and false is now true 🤔
Should we just switch the condition to archive the "old" result(s)?

@ST-DDT
Copy link
Member Author

ST-DDT commented Oct 22, 2022

Looking into the test snapshots, it looks like the behavior exactly swapped, true is now false and false is now true 🤔 Should we just switch the condition to archive the "old" result(s)?

IMO the probability should refer to true and not false. I dont know how to preserve the old behavior that wouldn't result in bad code readability/understandability.
E.g. 1 - float(...) < probability.
Its a major version upgrade so a predictably flipped random boolean is acceptable IMO.
Also, if we flip the result, then we would have to flip the usages as well (maybe+iban).

Shinigami92
Shinigami92 previously approved these changes Oct 24, 2022
src/modules/datatype/index.ts Show resolved Hide resolved
@ST-DDT ST-DDT dismissed stale reviews from xDivisionByZerox and Shinigami92 via 5421688 November 1, 2022 23:06
@ST-DDT ST-DDT force-pushed the feat/boolean-probability branch from 705ca55 to 6a66cc8 Compare November 1, 2022 23:25
src/modules/datatype/index.ts Outdated Show resolved Hide resolved
src/modules/datatype/index.ts Outdated Show resolved Hide resolved
ST-DDT and others added 2 commits November 2, 2022 09:32
@ST-DDT ST-DDT requested review from a team November 2, 2022 18:09
@ST-DDT ST-DDT merged commit 838f836 into next Nov 3, 2022
@ST-DDT ST-DDT deleted the feat/boolean-probability branch November 3, 2022 19:37
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: datatype Something is referring to the datatype 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