Skip to content
This repository has been archived by the owner on Dec 11, 2020. It is now read-only.

Update Generator.php #632

Merged
merged 1 commit into from
Jul 15, 2015
Merged

Update Generator.php #632

merged 1 commit into from
Jul 15, 2015

Conversation

theofidry
Copy link
Contributor

Cast $seed instead of checking if null. This way it does not throw an error if we passed a stringified int value either.

@@ -184,7 +184,7 @@ public function seed($seed = null)
if ($seed === null) {
mt_srand();
} else {
mt_srand($seed);
mt_srand(intval($seed));
Copy link
Contributor

Choose a reason for hiding this comment

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

no trailing whitespace please

Copy link
Contributor

Choose a reason for hiding this comment

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

Why not just use (int) $seed. It's exactly the same, but has better performance...

@theofidry
Copy link
Contributor Author

Thanks @GrahamCampbell, sorry for the trailing spaces :)

@@ -184,7 +184,7 @@ public function seed($seed = null)
if ($seed === null) {
mt_srand();
} else {
mt_srand($seed);
mt_srand((int)$seed);
Copy link
Contributor

Choose a reason for hiding this comment

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

Should be (int) $seed.

@fzaninotto
Copy link
Owner

An assertTrue() after your statement will do.

@theofidry
Copy link
Contributor Author

An empty one?

@fzaninotto
Copy link
Owner

Include a message explaining why you do this, something like:

$this->assertTrue(true, 'seeding with a string doesn\'t throw an exception');

Unfortunately , that's all phpUnit offers us to test that (cf sebastianbergmann/phpunit-documentation#171).

@theofidry
Copy link
Contributor Author

ping @fzaninotto fast merge :)

fzaninotto added a commit that referenced this pull request Jul 15, 2015
@fzaninotto fzaninotto merged commit b1abe41 into fzaninotto:master Jul 15, 2015
@fzaninotto
Copy link
Owner

Thanks!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants