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(number): move methods to new module #1122

Merged
merged 56 commits into from
Nov 25, 2022
Merged

Conversation

xDivisionByZerox
Copy link
Member

@xDivisionByZerox xDivisionByZerox commented Jul 1, 2022

This move all number generators into their own (number) module.

Idea for this is from #805.

@xDivisionByZerox xDivisionByZerox added c: feature Request for new feature p: 1-normal Nothing urgent s: accepted Accepted feature / Confirmed bug labels Jul 1, 2022
@xDivisionByZerox xDivisionByZerox added this to the vFuture milestone Jul 1, 2022
@xDivisionByZerox xDivisionByZerox requested a review from a team as a code owner July 1, 2022 10:16
@xDivisionByZerox xDivisionByZerox self-assigned this Jul 1, 2022
@xDivisionByZerox
Copy link
Member Author

Currently, I have removed all functions that were moved to the number module. I guess that wasn't the best move from me since we should deprecate them in v8 and remove them in v9. Correct?

@xDivisionByZerox xDivisionByZerox marked this pull request as draft July 1, 2022 10:21
src/modules/number/index.ts Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Jul 1, 2022

Codecov Report

Merging #1122 (237b81d) into next (0af0fff) will increase coverage by 0.00%.
The diff coverage is 98.59%.

Additional details and impacted files
@@           Coverage Diff            @@
##             next    #1122    +/-   ##
========================================
  Coverage   99.63%   99.64%            
========================================
  Files        2220     2221     +1     
  Lines      238957   239065   +108     
  Branches     1035     1045    +10     
========================================
+ Hits       238095   238209   +114     
+ Misses        841      835     -6     
  Partials       21       21            
Impacted Files Coverage Δ
src/modules/image/providers/lorempixel.ts 91.30% <0.00%> (ø)
src/modules/internet/user-agent.ts 93.24% <86.20%> (+0.65%) ⬆️
src/faker.ts 95.08% <100.00%> (+0.03%) ⬆️
src/index.ts 100.00% <100.00%> (ø)
src/modules/color/index.ts 99.73% <100.00%> (-0.01%) ⬇️
src/modules/commerce/index.ts 100.00% <100.00%> (ø)
src/modules/datatype/index.ts 100.00% <100.00%> (+0.81%) ⬆️
src/modules/date/index.ts 99.13% <100.00%> (ø)
src/modules/finance/index.ts 100.00% <100.00%> (ø)
src/modules/git/index.ts 100.00% <100.00%> (ø)
... and 9 more

@xDivisionByZerox
Copy link
Member Author

Suggestion from a discord user:

I would love to have a number.int, number.bigInt and something with IEEE 754

@xDivisionByZerox xDivisionByZerox force-pushed the feat/modules/number branch 2 times, most recently from bb10dc1 to 8ae5528 Compare July 25, 2022 21:48
@Shinigami92 Shinigami92 changed the title feat(modules): number feat(number): move methods to new module Aug 12, 2022
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.

BLOCKED: we cannot name the module Number as this is the JavaScript global Number constructor and we would run into the same issues as with _Date module (were we actually also want to find a better name for, like e.g. DateModule or Temporal)

If we name it NumberModule please first merge #932

@import-brain import-brain added the s: on hold Blocked by something or frozen to avoid conflicts label Aug 13, 2022
@xDivisionByZerox xDivisionByZerox force-pushed the feat/modules/number branch 2 times, most recently from 160f690 to f28b146 Compare August 18, 2022 20:53
@ST-DDT
Copy link
Member

ST-DDT commented Nov 23, 2022

Correction
Log10(2.5)

... well, we have a problem there:

image

So beside that I absolutely do not understand this math magic you are doing with these log10s, it results in values greater then the requested max 👀

Math: 1 / x = 0.4 => x = 2.5 and 10 ** log10(x) = x

Range: The issue here is probably with number.int
It uses +1 to make max inclusive.

1.9 × 2.5 = 4.75
4.75 + 1 = 5.75 (5 as int)
5 / 2.5 = 2

So instead of adding one we might want to add a very small number and then round up.
Alternatively substract precision from max before passing it to int. Not sure whether that creates other issues.

@ST-DDT ST-DDT closed this Nov 23, 2022
@ST-DDT ST-DDT reopened this Nov 23, 2022
@ST-DDT
Copy link
Member

ST-DDT commented Nov 23, 2022

We might want to be more strict in number.int anyway and ceil min and floor max before checking and using it. Because then we would pass 5 to mersenne, but since it is exclusive we will only get 4 as max returned value => 4 / 2.5 = 1.6.
Not tested yet.

@ST-DDT
Copy link
Member

ST-DDT commented Nov 23, 2022

Also since that math confuses even our own developers how about having both options?
Precision as used previously and decimal digits as used here?

@Shinigami92
Copy link
Member

I ensured together with @ST-DDT that the tests / bugs are already there before this PR and so I extracted it to #1595 so we do not pollute this PR with fixing this bug.

@Shinigami92
Copy link
Member

Due to the confusion with the log10 stuff, @ST-DDT and I decided for now to revert the parameter precision change from this PR.
But we will create a new issue to re-think about the change in another PR. (rename precision to fractionDigits)

Shinigami92
Shinigami92 previously approved these changes Nov 24, 2022
src/modules/commerce/index.ts Show resolved Hide resolved
src/modules/datatype/index.ts Show resolved Hide resolved
src/modules/datatype/index.ts Show resolved Hide resolved
src/modules/number/index.ts Show resolved Hide resolved
@ST-DDT ST-DDT requested review from a team November 24, 2022 19:52
@Shinigami92 Shinigami92 requested review from a team November 24, 2022 20:19
@Shinigami92 Shinigami92 merged commit 7d4d99f into next Nov 25, 2022
@Shinigami92 Shinigami92 deleted the feat/modules/number branch November 25, 2022 15:59
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 deprecation A deprecation was made in the PR 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.

Move Number methods to own module
4 participants