-
Notifications
You must be signed in to change notification settings - Fork 29
PHP 7.2 support, drop HHVM and PHP 5.5 + docs rebase #78
Conversation
With version 2 package has been renamed from "satooshi/php-coveralls" to "php-coveralls/php-coveralls", and the script has been renamed from "coveralls" to "php-coveralls"
- remove docs deploy - removed HHVM and nightly builds - added PHP 7.2 builds - removed IRC notifications - removed updating composer - set composer output width to 120 chars
- documentation directory changed to docs - updated copyright year range and https link to zend.com
- updated coveralls badge - updated link to the docs
Thanks for this PR. For my personal liking these are too many things in one PR! Adding PHP7.2 is one thing, Removing HHVM and PHP5.5 a second, rebasing the docs is a third and updating PHPUnit is a fourth. Would you mind splitting that up into several PRs? |
@heiglandreas All these changes are connected, if I start splitting it into separate PRs I'll create 4 conflicted PRs and then it will be even harder to merge it. For example I can't update to PHP 7.2 without updating PHPUnit. Please have a look on changes and you will notice that there is nothing surprising and reviewing it is quite simply. PR now is ready to merge, tests pass and the main goal is reached - PHP 7.2 support. |
AFAIK PHPUnit update doesn't need PHP7.2 but can be done regardeless of that. And dropping PHP5.5 and HHVM is unrelated to the rest as well as the docs part. Having 4 PRs that are based on one another is also a possibility. Especially as the related Issue is only about the PHP7.2 support. |
@heiglandreas One more time: I can't add PHP 7.2 support without update PHPUnit - PHPUnit 4 is not compatible with PHP 7.2. I can drop PHP 5.5 and HHVM in separate PR but then I have to update there also travis configuration and it will be conflicted. Sorry, I did tons PRs like that and I think it's much simpler to have it all together tested and confirmed that all is good. You can see for example:
If you are the maintainer of the repository and you are not going to accept this PR feel free to close it. I'm not going to split it into smaller PRs just to have tons of conflicted PRs and multiple issues with resolving these conflicts. |
In my opinion, we should not make the work for @webimpress more complicated! |
Thanks a lot. I'm happy that these is still someone who can understand me... |
@webimpress Thanks again for this PR. I'll review it later today anyhow. And I'm absolutely understanding you that it is easier to do thins in one PR. Nevertheless there are a lot of changes cluttered into one file regarding different tasks. @froschdesign the changes do not only affect the infrastructure. Otherwise there wouldn't be changes to tests that need to be reviewed. Without them I wouldn't bother. |
And I would be able to review small PRs without an issue in between some tasks. For this PR I need dedicated time due to the size… |
@heiglandreas Again: feel free to close it and wait until somebody else will do it as you want. I'm sure you will lose more time on merging and resolve conflict with smaller conflicted PRs than reviewing that one, where there is nothing really to review... |
It really make me sad that it is necessary to get personal when all we are talking about here is the code. If you feel that suggesting splitting up a PR concerning more than 40 files and including separate unrelated topics is a personal insult then I'm really sorry about that. It has nothing to do with you as a person but with the content of the PR. And I think I tried to explain why I suggested splitting up the PR from a technical POV. And it was a suggestion, not a need to do. |
@heiglandreas check the files and changes, not the number of files... We are wasting time on arguing about nothing, when all changes are straight forward. |
@heiglandreas Two fixes in code: Changes in test:
Docs, Readme, Travis, Composer, PHPUnit etc.:
And all is done in separate commits. |
@heiglandreas |
PHP 7.2 support, drop HHVM and PHP 5.5 + docs rebase
Resolves #74
count
fix - 633243f)This PR is based on #76 but with develop target.