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

feat: allow friendsofphp/php-cs-fixer >= 3.10 #2626

Merged

Conversation

Chris53897
Copy link
Contributor

@Chris53897 Chris53897 commented Jun 7, 2023

bump "rector/rector" to 0.17

is the upper constraint for friendsofphp/php-cs-fixer still in need?

On local development, this blocks doctrine/annotations v2.
The CI removes it before running the tests.
https://github.com/doctrine-extensions/DoctrineExtensions/blob/main/.github/workflows/continuous-integration.yml#L65

Copy link
Collaborator

@phansys phansys left a comment

Choose a reason for hiding this comment

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

I'm fine allowing higher versions of friendsofphp/php-cs-fixer as long the considerations mentioned at #2507 regarding the phpdoc_order rule are covered.

@codecov-commenter
Copy link

codecov-commenter commented Jun 17, 2023

Codecov Report

Attention: 1 lines in your changes are missing coverage. Please review.

Comparison is base (2d327ad) 79.36% compared to head (84eced7) 79.39%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2626      +/-   ##
==========================================
+ Coverage   79.36%   79.39%   +0.02%     
==========================================
  Files         162      162              
  Lines        8457     8454       -3     
==========================================
  Hits         6712     6712              
+ Misses       1745     1742       -3     
Files Coverage Δ
src/AbstractTrackingListener.php 94.54% <ø> (ø)
src/Blameable/Mapping/Driver/Annotation.php 80.00% <100.00%> (ø)
src/Blameable/Traits/BlameableDocument.php 0.00% <ø> (ø)
src/Blameable/Traits/BlameableEntity.php 100.00% <ø> (ø)
src/IpTraceable/Mapping/Driver/Annotation.php 77.77% <100.00%> (ø)
src/IpTraceable/Traits/IpTraceableDocument.php 0.00% <ø> (ø)
src/IpTraceable/Traits/IpTraceableEntity.php 100.00% <ø> (ø)
src/Loggable/LoggableListener.php 92.74% <ø> (ø)
src/Mapping/Annotation/Blameable.php 100.00% <ø> (ø)
src/Mapping/Annotation/IpTraceable.php 35.71% <ø> (ø)
... and 48 more

... and 1 file with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@Chris53897
Copy link
Contributor Author

Chris53897 commented Jun 18, 2023

Ok i see a lot of default rules changed. I had the same problem at another repo.

Can you please decide how you want this configs to be set.
phpdoc_separation
global_namespace_import @var \DateTime|null vs @var DateTime|null

I have added no_superfluous_phpdoc_tags => false, because it would otherwise remove useful phpdocs (in my point of view).
Additional i have added a composer-script to check it locally (dry-run same as in ci) to make it easier to check it local.

I run it locally on php 8.2. Looks like the default is correct followed

'phpdoc_order' => [
                'order' => [
                    'param',
                    'return',
                    'throws',
                ],
            ],

@franmomu franmomu closed this Aug 19, 2023
@franmomu franmomu reopened this Aug 19, 2023
@franmomu
Copy link
Collaborator

Can you please decide how you want this configs to be set. phpdoc_separation global_namespace_import @var \DateTime|null vs @var DateTime|null

Now I see this, I think we can use for phpdoc_separation:

'phpdoc_separation' => ['groups' => [
            ['Gedmo\\*'],
            ['ODM\\*'],
            ['ORM\\*'],
        ]],

global_namespace_import I would leave it like @var \DateTime|null which is what we have now.

I have added no_superfluous_phpdoc_tags => false, because it would otherwise remove useful phpdocs (in my point of view). Additional i have added a composer-script to check it locally (dry-run same as in ci) to make it easier to check it local.

I run it locally on php 8.2. Looks like the default is correct followed

'phpdoc_order' => [
                'order' => [
                    'param',
                    'return',
                    'throws',
                ],
            ],

About the order, we have now ['param', 'throws', 'return'] which was the default order, I think @phansys meant to keep it

@phansys phansys requested a review from franmomu August 19, 2023 22:16
composer.json Outdated Show resolved Hide resolved
@Chris53897
Copy link
Contributor Author

Anything todo for me here?

@phansys
Copy link
Collaborator

phansys commented Aug 20, 2023

Anything todo for me here?

No @Chris53897, thank you.
I'll be waiting for the @franmomu's review.

composer.json Outdated Show resolved Hide resolved
@franmomu franmomu merged commit 4071f71 into doctrine-extensions:main Oct 29, 2023
19 checks passed
@franmomu
Copy link
Collaborator

Thanks @Chris53897! and @phansys for reviewing!

@Chris53897 Chris53897 deleted the feature/bump-dependencies branch February 19, 2024 19:44
paxal pushed a commit to sportmium/DoctrineExtensions that referenced this pull request Mar 18, 2024
* feat: allow friendsofphp/php-cs-fixer >= 3.10, bump "rector/rector" to 0.17

* feat: allow friendsofphp/php-cs-fixer >= 3.10, bump "rector/rector" to 0.17

* Configure "phpdoc_order" and "phpdoc_separation" CS rules

* Apply CS

* Apply PHP-CS-fixer

---------

Co-authored-by: Christopher Georg <[email protected]>
Co-authored-by: Fran Moreno <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants