Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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/unstable): basic randomization functions #5626
feat(random/unstable): basic randomization functions #5626
Changes from 17 commits
19b6ed7
f5fa351
09b2861
7d3e287
1200129
9208fe1
d39753c
82a0c00
276cc3c
6dc1b3f
8bbd63e
3f55d14
b501ced
ae7ef2a
79a694d
1196193
c86aadb
a3cc9df
333d07d
7e1538f
793f090
2a96137
e472f71
810d026
2df294e
b3cda5d
bbe382e
f81a57b
a4cf0ce
e8e1b33
03c36fb
371e73f
c7d3c53
7e6504c
bf9b6d8
03fbfed
0156a3a
21b1a24
4d95110
0c912c7
be0d89a
c2c934a
e501043
dbb7f94
18442d6
8980fec
3a80b0e
003d2ac
f226f4c
9dcb837
db2477f
82cc66f
d790759
0088886
6b4b79a
6bb99e4
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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 option is not very web api-ish. I think it would be better to remove it and add some kind of
algorithm
option if needed, that calls different functions internally. DoingcustomRandomFunction() * (max - min) + min
seems very trivial.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.
Callbacks seem pretty web API-ish to me, and they're more versatile than specifying a name of an algorithm — with a callback, it's easy to use with third-party randomization functions/sources. But perhaps more importantly, specifying an algorithm name would surely make any and all of the all implemented algorithms into non-tree-shakeable dependencies of any function taking such options.
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.
@kt3k do we have a guide on how to name option functions? I thought thy have a suffix of
Fn
->randomFn
. Not sure though.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.
Let's make this an interface that uses the suggested
Prng
type as an optional property.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.
This doesn't seem needed.
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.
Please remove. We can just use
Math.random()
in implementations, as it's more explicit.