refactor: replace pseudo-random number generator #1479
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Codecov Report
@@ Coverage Diff @@
## master #1479 +/- ##
==========================================
+ Coverage 33.91% 33.96% +0.04%
==========================================
Files 248 248
Lines 9182 9169 -13
Branches 1986 1985 -1
==========================================
Hits 3114 3114
+ Misses 5464 5451 -13
Partials 604 604
Continue to review full report at Codecov.
|
| getVariantInfoDimensions.mockImplementation(() => dimensions) | ||
| }) | ||
| describe('on mount', () => { | ||
| it('connects to drag', () => { | ||
| shallow(<SegmentForPalette connectDropTarget={connectDropTarget} connectDragSource={connectDragSource} connectDragPreview={connectDragPreview} type={''} variantString={''} intl={intl} />) | ||
| shallow(<SegmentForPalette connectDropTarget={connectDropTarget} connectDragSource={connectDragSource} connectDragPreview={connectDragPreview} type={''} variantString={''} intl={intl} randSeed={42} />) |
There was a problem hiding this comment.
"The Answer to the Ultimate Question of Life, The Universe, and Everything."
|
|
||
| for (let i = 1; i < floors; i++) { | ||
| const variant = (building.variantsCount === 0) ? 0 : Math.floor(randomGenerator.rand() * building.variantsCount) + 1 | ||
| const variant = (building.variantsCount === 0) ? 0 : Math.floor(randomGenerator() * building.variantsCount) + 1 |
There was a problem hiding this comment.
🤔 part of me wonders if we should explore using UUIDs, but that's definitely out of scope here
whitmanschorn
approved these changes
Aug 13, 2019
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
I've been doing a lot of learning on how seeding pseudo-random number generation works, and looked into how we implement it here, which has been largely untouched (besides some language refactors) since 2013. In particular, I wanted to know why the random seeds are occasionally modified with magic numbers (and the short answer is: no good reason, at least not anymore, so we can remove those parts of the code).
Furthermore I found the
seedrandompackage which seems to be really well made as a PRNG utility, so in this PR I propose we start using it. There is one more dependency as a result, but the alternative is to vendor it into the code directly, and in any case I don't think we need our own implementation of it (however simple). The other tradeoff from migrating to this package is that it does change the result of existing random seeds. It only affects the layout of people on sidewalks right now, and if there is anyone out there that was relying on specific seeds for a specific mix of people they're in for a bad time. But I think that'll be a really rare case and in people can always re-roll pedestrians.