FriendsOfSymfony1 / doctrine1

[DEPRECATED -- Use Doctrine2 instead] Doctrine 1 Object Relational Mapper.
http://www.doctrine-project.org
GNU Lesser General Public License v2.1
40 stars 75 forks source link

add php cs fixer #112

Open connorhu opened 9 months ago

connorhu commented 9 months ago

kudos to mlocati and his magnificent tool: https://mlocati.github.io/php-cs-fixer-configurator/

connorhu commented 9 months ago

I will rollback the migration commits, but i had some time to play with the rules.

connorhu commented 9 months ago

I did this PR, but now I'm thinking that it's actually the code base doctrine, so it would be closer to the doctrine-current project if we use the doctrine specific PHP_CodeSniffer here: https://github.com/doctrine/coding-standard

thePanz commented 9 months ago

I did this PR, but now I'm thinking that it's actually the code base doctrine, so it would be closer to the doctrine-current project if we use the doctrine specific PHP_CodeSniffer here: https://github.com/doctrine/coding-standard

I would keep this PR for the following reasons:

WDYT @connorhu ?

connorhu commented 8 months ago

I did this PR, but now I'm thinking that it's actually the code base doctrine, so it would be closer to the doctrine-current project if we use the doctrine specific PHP_CodeSniffer here: https://github.com/doctrine/coding-standard

I would keep this PR for the following reasons:

* maintaining symfony1 and doctrine1 with the same CS will reduce the costs of context switching

In some cases, the cs-fixer will break the original formatting (which is what the current doctrine uses) rather than improve it. For example, the equals sign in the value of variables has been indented since the beginning. This is how they do it now, and this formatting is not done by cs-fixer as far as I know.

The more I see of cs-fixer, the more I think that using cs-fixer in the symfony1 project was a bad choice. For example, the developers of cs-fixer just want to fix the code, they do not want to serve reporting purposes. That's why the annotation location is buggy. CodeSniffer by design solves this and it works fine. The current-symfony sweeps this problem under the rug instead and is one of the reasons why fabbot.io exists.

* the PR [add doctrine/coding-standard #123](https://github.com/FriendsOfSymfony1/doctrine1/pull/123) is failing, not sure how much effort is needed to fix all 4000 errors 🙈

I see no problem with turning off all problematic rule and turning them back on one by one. The same should be done with the psalm, I'm not short of tasks there either. I did the port of the symfony1 test system to phpunit, that was no small job. (Btw there are a few bugs I fixed and haven't put in the patch yet.)

connorhu commented 8 months ago

kép

This is an example of how codesniffer works well. By the way, I don't agree with all doctrine style rules (not even the one I took the picture of, I've written if (!$variable and not if (! $variable all my life). If a style set is set up carefully then whoever writes code doesn't really need to pay attention, running a fix command before committing is not an issue.

connorhu commented 8 months ago

@thePanz @thirsch Seeing how much misunderstanding php-cs-fixer causes, I don't see it as suitable for CI/QA purposes for the time being. How about we drop cs-fixer here now, introduce php-codesniffer and when it is suitable for quality reporting we will switch to cs-fixer. https://github.com/FriendsOfSymfony1/doctrine1/pull/123 is green now.

Having heard @thirsch's comment on swiftmailer the other day, I also recommend the code sniffer for less change noise.