hamcrest / hamcrest-php

PHP Hamcrest implementation [Official]
Other
6.96k stars 44 forks source link

Removing php 7 from the allowed failure #32

Closed mikeSimonson closed 8 years ago

aik099 commented 8 years ago

Please rebase, because a fix has been committed to fix Composer error, that is happening.

mikeSimonson commented 8 years ago

@aik099 done

aik099 commented 8 years ago

I see more changes, than noted in PR description. What are they about?

The PR must stay on topic.

mikeSimonson commented 8 years ago

@aik099 After all the changes you proposed there are 2 issues left.

aik099 commented 8 years ago

It needs the is_string otherwise it fail when it can't cast to string (object without toString).

OK.

And the last issue is to decide if you want to consider the case "0x" as a valid representation of 0 in hexadecimal or not ?

If it's valid in PHP, then it is for this library as well. I wrote this in the relevant test case.

aik099 commented 8 years ago

Today’s review completed.

aik099 commented 8 years ago

@mikeSimonson , please reply to each inline comment (also from outdated diffs) to indicate if that was fixed or not.

Since GitHub doesn't send notifications on new commits I also can't review your changes unless you add a comment to the issue about that.

mikeSimonson commented 8 years ago

Unless I missed something it should be done. @aik099

aik099 commented 8 years ago

Looks good, except few CS issues I've described above.

Please also squash and rebase.

mikeSimonson commented 8 years ago

@aik099 Done

mikeSimonson commented 8 years ago

@aik099 re done

aik099 commented 8 years ago

Merging. Thanks @mikeSimonson .

aik099 commented 8 years ago

@davedevelopment , this PR is merged. You can make a 2.0.0 version release when you're ready.