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

Change seeds so they work the first time they are passed in #48

Merged
merged 2 commits into from
Nov 4, 2015

Conversation

daviesgeek
Copy link
Contributor

Bug Report

Currently the way randomColor works, a seed will work the very first time it's passed in, but if the seed changes, it uses the last seed that was passed in to generate the color(s), and then it sets the new seed, causing the wrong output.

Here's an example:

randomColor({seed: 1, count: 10})
// Output:
['#8bd343', '#a31f44', '#f23aa5', '#25bfd1', '#b9dd5d', '#b2ffd3', '#017984', '#c4214f', '#fcd928', '#55dbd2']


randomColor({seed: 2, count: 10}) // Note the seed has changed from 1 to 2
// Notice that this is the same array as when it was seeded with "1"
['#8bd343', '#a31f44', '#f23aa5', '#25bfd1', '#b9dd5d', '#b2ffd3', '#017984', '#c4214f', '#fcd928', '#55dbd2']

randomColor({seed: 2, count: 10}) // Note the seed is still 2
// This is now the correct array of colors
['#7ed65e', '#d8d82f', '#46e2a1', '#77eaea', '#d062fc', '#8e6af2', '#604ce0', '#e5d177', '#9df2e2', '#efcb8d']

As you can see, when you run the generator with one seed (first run), change the seed, run it again (second run), it generates the wrong array of colors. Then when you run it for the third time (with the
same seed as the second run), it generates the correct array of colors.

Solution

This pull request modifies the behavior of the seed option so that the seed is reset at the end of each run.

I've also written a test that fails if you use the original version of randomColor (before my changes), so that you can run it (via npm run test) and easily see the bug.

Misc

I've also bumped the version number in package.json, as well as added a version number to the bower.json file. I also created a new tag for the 0.4.1 release (not sure if this will move over when my PR is merged in though…)

@davidmerfield
Copy link
Owner

Hi Matthew! Thank you for your work in fixing this bug. I'd be happy to merge this if you remove the tests & associated changes from this pull request.

@kimmobrunfeldt
Copy link

@davidmerfield why do you not want tests for the project?

@daviesgeek
Copy link
Contributor Author

Whoops! So sorry! Somehow I missed your comment.

I am also wondering why you would not want tests for the project. Do you mind explaining why?

@davidmerfield
Copy link
Owner

@kimmobrunfeldt @daviesgeek It's not that I don't want to use tests – I don't want to use Karma/Jasmine. I'd rather move the question of how to add tests to this project to a separate issue/pull request. It is much easier to manage single-problem pull requests. Again, thank you for your work fixing this bug and apologies for my delayed response!

Added version number to bower.json
@daviesgeek
Copy link
Contributor Author

Okay I've updated the PR to remove the unit tests!

davidmerfield added a commit that referenced this pull request Nov 4, 2015
Change seeds so they work the first time they are passed in
@davidmerfield davidmerfield merged commit 6c4a5e2 into davidmerfield:master Nov 4, 2015
@davidmerfield
Copy link
Owner

Perfect, thank you again!

@daviesgeek daviesgeek mentioned this pull request Nov 4, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants