Enhancement: Use friendsofphp/php-cs-fixer instead of StyleCI #157
Conversation
localheinz
force-pushed
the
feature/php-cs-fixer
branch
from
9828b61 to
67a4fde
Compare
|
I agree with this to not be dependant on external. Running it locally is also a big advantage |
|
👎 here. We are just adding more maintenance for us here. We should stop messing with the code style, and just leave StyleCI to enforce it. Contributors and maintainers alike should not ever be running code style tooling. |
This is an advantage. Nobody should be wasting their time with that. |
|
Though, StyleCI can be run locally, if really wanted (https://github.com/StyleCI/CLI). |
I prefer to use and run But, I understand your opinion and your argumentation - after all, StyleCI is your pet project. |
| { | ||
| "require": { | ||
| "php": "^7.1 || ^8.0", | ||
| "friendsofphp/php-cs-fixer": "^2.17.2" |
|
I agree with it being more transparant to run php-cs-fixer. The idea of relying on an external service always worries me a bit personally. I don't agree with the fact that developers shouldn't be doing code style. Using editorconfig file, phpstan and php-cs-fixer/StyleCI should be part of the IDE workflow. Adding our own code to commit php-cs-fixer changes in GH Actions feels like too much overhead, though. My opinion is that a PR owner should be responsible for not following code style, not an external service. This should also make the build fail. |
localheinz
requested review from
pimjansen,
Nyholm,
aanfarhan,
chris-doehring,
driesvints,
icanhazstring,
sanmai and
tomasnorre
|
I am also in favor for php-cs-fixer as direct a dev dependency. The PR should be blocked unless everything is green. Changing any code afterwards seems weird to me. I on my part only allow valid code to be merged into the main branch of repositories. So fine my me 👍 |
There was a problem hiding this comment.
The code looks fine.
I don't really mind either way. PHP-CS-fixer may have some benefits, but the same is true with StyleCI. If I were to decide, I would have solved this in a third way...
However, I feel like I am a better resource if I focus on code changes.
There was a problem hiding this comment.
I would always prefer a Style Checker locally. I want to provide as clean as possible code when pushing to what ever repository.
I think it's important to see the changes done by a code style checker to learn from it. A lot of people just ignores code style, and if the CI does it for you, you will more and more often ignore what code you actually write.
My preferred option is having the phpcs for locally and what ever tool for pipeline taking care of the fixes not already done, but the two tools should follow the exact same ruleset of course.
|
I see the benefit's of both strategies, but I also prefer using a "native" local tool for the following reasons:
|
|
Quick heads-up, after merging this pull request, StyleCI applied the following changes: Looks like a failure to me, to apply fixes when a configuration has been removed, and a great example for a violation of the principle of least astonishment as well. |
|
This is because StyleCI is still enabled on the repo, and uses the default configuration when none is provided. This is what people expect to happen, based on user feedback. The majority of StyleCI users never set any config. |
| strategy: | ||
| matrix: | ||
| php-version: | ||
| - 7.1 |
There was a problem hiding this comment.
Should this be something like 7.4?
I'm late to the party, true.
| uses: shivammathur/setup-php@v2 | ||
| with: | ||
| coverage: none | ||
| extensions: intl |
| uses: actions/cache@v2 | ||
| with: | ||
| path: .php_cs.cache | ||
| key: composer-${{ runner.os }}-${{ matrix.php-version }}-${{ hashFiles('composer.*') }} |
There was a problem hiding this comment.
composer.lock will be all new after each composer update, so this cache might not work as expected.
There was a problem hiding this comment.
And if we consider vendor-bin this should be hashFiles('**/composer.*')
| }, | ||
| "config": { | ||
| "platform": { | ||
| "php": "7.1.33" |
This PR
friendsof/php-cs-fixerand removes the integration with StyleCIStyleCI
The integration with StyleCI has the following advantages:
main.The integration with StyleCI has the following disadvantages:
A developer can not run StyleCI locally.A developer can run StyleCI locally only by installing a separate package, which however, requires PHP 7.2. We currently support PHP 7.1.friendsofphp/php-cs-fixerunder the hood, but adds obscurity on top. For example, it "invents" new fixers that can be enabled or enabled, but are in reality different configurations for existing fixers). Additionally, fixers that are available today infriendsofphp/php-cs-fixercannot be configured in StyleCI without delay (if at all). Furthermore, we do not know which version offriendsofphp/php-cs-fixeris used under the hood.friendsofphp/php-cs-fixerRequiring
friendsofphp/php-cs-fixeras a dependency (similar tophpstan/phpstanandvimeo/psalm) alleviates all of these problems. We can use and configure any fixers that is available any time we want.If we wanted to automatically apply coding standard issues (similar to StyleCI), we could easily do that with GitHub Actions. However, I would suggest to refrain from it.
In my opinion, a pull request needs to be in mergeable state before merging it into the mainline of development, and this includes coding standard issues.