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: random numeric #797

Merged
merged 2 commits into from
Apr 24, 2022
Merged

feat: random numeric #797

merged 2 commits into from
Apr 24, 2022

Conversation

Shinigami92
Copy link
Member

@Shinigami92 Shinigami92 commented Apr 7, 2022

This introduce a new function beside alpha and alphaNumeric in the random module.
This function will later be used as a base for other faker functions like finance.pin and datatype.bigInt (#791).
It can also be used for potential new faker functions like internet.authCode.

@Shinigami92 Shinigami92 added c: feature Request for new feature needs test More tests are needed p: 1-normal Nothing urgent s: accepted Accepted feature / Confirmed bug labels Apr 7, 2022
@Shinigami92 Shinigami92 added this to the v6.2 - New small features milestone Apr 7, 2022
@Shinigami92 Shinigami92 self-assigned this Apr 7, 2022
@codecov
Copy link

codecov bot commented Apr 7, 2022

Codecov Report

Merging #797 (2915439) into main (f797b63) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main     #797   +/-   ##
=======================================
  Coverage   99.42%   99.42%           
=======================================
  Files        1958     1958           
  Lines      210804   210862   +58     
  Branches      905      919   +14     
=======================================
+ Hits       209590   209649   +59     
+ Misses       1157     1156    -1     
  Partials       57       57           
Impacted Files Coverage Δ
src/random.ts 99.50% <100.00%> (+0.05%) ⬆️
src/index.ts 97.50% <0.00%> (+2.50%) ⬆️

@Shinigami92 Shinigami92 removed the needs test More tests are needed label Apr 7, 2022
@Shinigami92 Shinigami92 marked this pull request as ready for review April 7, 2022 11:48
@Shinigami92 Shinigami92 requested a review from a team as a code owner April 7, 2022 11:48
@Shinigami92 Shinigami92 requested review from ST-DDT and a team April 7, 2022 11:48
src/random.ts Outdated Show resolved Hide resolved
src/random.ts Outdated Show resolved Hide resolved
src/random.ts Outdated Show resolved Hide resolved
src/random.ts Outdated Show resolved Hide resolved
@import-brain
Copy link
Member

Doesn't this do the same thing as finance.pin()?

src/random.ts Outdated Show resolved Hide resolved
@Shinigami92
Copy link
Member Author

Doesn't this do the same thing as finance.pin()?

@import-brain: a little bit yes, but it is more like finance.pin can use this function. For the UX I would like to have both functions, but with a limited option set that is specialised for generating pin numbers. Maybe we also could add something like internet.authCode?
datatype.bigInt will also use this function.

@pkuczynski: @ST-DDT and I have short communication paths via personal friendship and discord, so we sometimes implement stuff together in our private space. I understand your suggestions, but sometimes we are a little bit ahead in thinking processes (e.g. in terms of how we want to re-structure the modules in "vFuture"). Not everything of that is somewhere to read because we are just thinking by communicating. So it might be that we want to go into another direction as you suggested.
But re-structuring modules will be anyways something for that far future.

@pkuczynski
Copy link
Member

I noticed that you have short paths of communication, which is fine of course, but keeping the community away from it, is kind of demotivating... :/

@ST-DDT
Copy link
Member

ST-DDT commented Apr 8, 2022

I noticed that you have short paths of communication, which is fine of course, but keeping the community away from it, is kind of demotivating... :/

Thanks for letting us know. We try to improve on this and establish regular team-meetings/be more open about it.
You can always ping us, if you have a specific topic you would like to discuss.
Things like this tend to come up during our reviews and it's hard to always communicate them.
(I usually tend to create issues for those things, but since this is only at the stage of we are thinking about it, I haven't done so yet)
I created a discussion for this idea of ours now: #805

If you notice again, that there are some things that haven't been communicated (well), then let us know.

@Shinigami92 Shinigami92 marked this pull request as draft April 8, 2022 09:05
@Shinigami92 Shinigami92 added the needs test More tests are needed label Apr 8, 2022
@Shinigami92 Shinigami92 marked this pull request as ready for review April 8, 2022 09:57
@Shinigami92 Shinigami92 requested review from ST-DDT and a team April 8, 2022 09:58
@Shinigami92 Shinigami92 removed the needs test More tests are needed label Apr 8, 2022
src/random.ts Outdated Show resolved Hide resolved
test/random.spec.ts Outdated Show resolved Hide resolved
test/random.spec.ts Outdated Show resolved Hide resolved
test/random.spec.ts Outdated Show resolved Hide resolved
test/random.spec.ts Outdated Show resolved Hide resolved
test/random.spec.ts Outdated Show resolved Hide resolved
@ST-DDT ST-DDT added the do NOT merge yet Do not merge this PR into the target branch yet label Apr 8, 2022
pkuczynski
pkuczynski previously approved these changes Apr 8, 2022
Copy link
Member

@pkuczynski pkuczynski left a comment

Choose a reason for hiding this comment

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

As suggested before I would put it in faker.string.numbers

import-brain
import-brain previously approved these changes Apr 10, 2022
Copy link
Member

@import-brain import-brain 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 this is fine where it is, don't think there's really a need to move it

@Shinigami92
Copy link
Member Author

I think this is fine where it is, don't think there's really a need to move it

Thx, I also would like to move the module discussion into a later major and not bind it to these feature implementations

@pkuczynski
Copy link
Member

pkuczynski commented Apr 10, 2022

Thx, I also would like to move the module discussion into a later major and not bind it to these feature implementations

Its a new function so it's not about moving it but rather putting in a future destination place... otherwise we will have to deprecate it and people will need to make an update on the next major release. Double the work. And seems that everybody agrees that datatype module should be gone...

@Shinigami92
Copy link
Member Author

Deprecations should not be removed directly, but marked as for removal or moved to new location

@Shinigami92
Copy link
Member Author

This PR adds a function in v6.2 to a module that already exists in v6
Adding a whole new module is a separate PR and targeted for v7 or later

@Shinigami92 Shinigami92 force-pushed the feat-random-numeric branch 2 times, most recently from 9edd2fb to 633f857 Compare April 20, 2022 12:37
@Shinigami92 Shinigami92 removed the do NOT merge yet Do not merge this PR into the target branch yet label Apr 20, 2022
@Shinigami92 Shinigami92 requested review from ST-DDT, import-brain and a team April 20, 2022 12:49
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.

Just some minor things left.

Also what about:
#797 (comment)

test/random.spec.ts Outdated Show resolved Hide resolved
test/random.spec.ts Show resolved Hide resolved
Co-authored-by: ST-DDT <ST-DDT@gmx.de>
@ST-DDT ST-DDT requested a review from a team April 22, 2022 12:07
@Shinigami92 Shinigami92 merged commit 712b1de into main Apr 24, 2022
@Shinigami92 Shinigami92 deleted the feat-random-numeric branch April 24, 2022 17:12
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 p: 1-normal Nothing urgent s: accepted Accepted feature / Confirmed bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants