search

tedious / JShrink

Javascript Minifier built in PHP
http://www.tedivm.com
BSD 3-Clause "New" or "Revised" License
749 stars 152 forks source link

PHP 7.4 refactoring: Use strict scalar types only with automated tests. #98

Closed janbarasek closed 1 year ago

janbarasek commented 3 years ago

Hi, I refactored the whole kernel of the package to conform to PHP 7.4 standards and use strict scalar data types.

BC break: The value false has been replaced by the modern null variant.

All other changes are backward compatible. At the same time, the package was ready for future improvements such as automated tests and more.

I test all the adjustments very carefully on hundreds of examples. The whole code has been tested for the strictest PhpStan test settings and fits perfectly.

Thanks! I love this package!

syteknet-core commented 3 years ago

@janbarasek one of the tests fail when running in your branch.

janbarasek commented 3 years ago

@syteknet-core I added an automatic test for PhpStan as GitHub Action and now it appears to be passed:

https://github.com/janbarasek/JShrink/runs/1526425900

How can I add other mandatory tests to the CI so that they always run automatically and there is a certainty that the commit does not break anything?

janbarasek commented 3 years ago

@syteknet-core Ok, I already know what the mistake was.

I added an automatic GitHub unit test, which will start automatically. Everything is okay now.

https://github.com/janbarasek/JShrink/runs/1526628460

Snímek obrazovky 2020-12-09 v 21 50 16

The error was resolved by this commit: https://github.com/tedious/JShrink/pull/98/commits/b89b5abc0f41b808e3981ed59b75da172a9a3af0

Thanks!

janbarasek commented 3 years ago

@tedivm :)

syteknet-core commented 3 years ago

@janbarasek - ok great news! we just need to add an instance with php-8.0 to the CI. also moving to the recent phpunit version makes sense. feel free to merge PR #99 wich already solves both!

janbarasek commented 3 years ago

@syteknet-core I think it's better to merge each branch independently, because each PR has to deal with exactly one thing.

syteknet-core commented 3 years ago

@janbarasek indeed but you have added automated testing to the scope of your PR by yourself. right now it is missing the testing instance for php-8.0 which will lead to the phpunit upgrade and feels incomplete.

tedivm commented 1 year ago

This project has recently upgraded its test suite to work with Github Actions and PHPUnit 10. If you are still interested in this PR please rebase and open a new PR. Thanks!