Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add php cs fixer #112

Open
wants to merge 19 commits into
base: master
Choose a base branch
from

Conversation

connorhu
Copy link
Collaborator

No description provided.

@connorhu connorhu force-pushed the feature/phpcsfixer branch 4 times, most recently from 4f02565 to d7fe67d Compare January 24, 2024 09:10
*
* @see Doctrine_Core::FETCH_* constants
* @param integer $fetchStyle Controls how the next row will be returned to the caller.
Copy link
Member

@thePanz thePanz Jan 24, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is weird: are those parameters not used or sub-implementations are defining them?

Another option is: should this class implement the interface below?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link

@alquerci alquerci Feb 9, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@thePanz I answered already on #114

.php-cs-fixer.dist.php Outdated Show resolved Hide resolved
@connorhu connorhu force-pushed the feature/phpcsfixer branch 3 times, most recently from 12797b5 to 2e5a8ee Compare January 24, 2024 18:58
@connorhu
Copy link
Collaborator Author

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

@connorhu
Copy link
Collaborator Author

connorhu commented Jan 24, 2024

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

@connorhu connorhu force-pushed the feature/phpcsfixer branch 6 times, most recently from 894a864 to 7b68440 Compare February 6, 2024 11:13
@connorhu connorhu marked this pull request as ready for review February 6, 2024 12:33
@connorhu
Copy link
Collaborator Author

connorhu commented Feb 6, 2024

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
Copy link
Member

thePanz commented Feb 7, 2024

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
  • the PR add doctrine/coding-standard #123 is failing, not sure how much effort is needed to fix all 4000 errors 🙈

WDYT @connorhu ?

composer.json Outdated Show resolved Hide resolved
@connorhu
Copy link
Collaborator Author

connorhu commented Feb 10, 2024

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
Copy link
Collaborator Author

connorhu commented Feb 10, 2024

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
Copy link
Collaborator Author

connorhu commented Feb 20, 2024

@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.
#123 is green now.

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants