-
-
Notifications
You must be signed in to change notification settings - Fork 939
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
feat: random numeric #797
Conversation
Codecov Report
@@ 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
|
Doesn't this do the same thing as |
@import-brain: a little bit yes, but it is more like @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. |
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. If you notice again, that there are some things that haven't been communicated (well), then let us know. |
960b4e4
to
6bad0b5
Compare
There was a problem hiding this 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
There was a problem hiding this 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
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 |
Deprecations should not be removed directly, but marked as for removal or moved to new location |
This PR adds a function in v6.2 to a module that already exists in v6 |
5f7e874
9edd2fb
to
633f857
Compare
9ac7561
to
c1711e5
Compare
There was a problem hiding this 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)
Co-authored-by: ST-DDT <ST-DDT@gmx.de>
c1711e5
to
296ac71
Compare
This introduce a new function beside
alpha
andalphaNumeric
in the random module.This function will later be used as a base for other faker functions like
finance.pin
anddatatype.bigInt
(#791).It can also be used for potential new faker functions like
internet.authCode
.