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: Add ordered_types to @Symfony #7356

Merged
merged 1 commit into from Feb 2, 2024

Conversation

VincentLanglet
Copy link
Contributor

Hi @Wirone

Symfony coding style already use:

'phpdoc_types_order' => [
                'null_adjustment' => 'always_last',
                'sort_algorithm' => 'none',
            ]

so I assume same should be applied to the types, ie:

'ordered_types' => [
                'null_adjustment' => 'always_last',
                'sort_algorithm' => 'none',
            ]

@VincentLanglet VincentLanglet changed the title Add ordered_types to SymfonySet feat: Add ordered_types to SymfonySet Oct 4, 2023
@coveralls
Copy link

coveralls commented Oct 4, 2023

Coverage Status

coverage: 94.751% (+0.001%) from 94.75%
when pulling 556d3eb on VincentLanglet:orderTypeSymfony
into d12282c on PHP-CS-Fixer:master.

@VincentLanglet
Copy link
Contributor Author

I'm not sure to understand the failures...

@SpacePossum
Copy link
Contributor

The failures are a side effect of your changes.
The test tests/Fixtures/Integration/misc/PHP8_2.test is run with the configuration @PhpCsFixer which contains/extends itself the Symfony set. Since you changed the SF set the new configuration is applied on the tests/Fixtures/Integration/misc/PHP8_2.test.

The diff now looks like:

-function falsyNull(null $n, false $f): null|false {}
+function falsyNull(null $n, false $f): false|null {}

Now this expected since you've changed the configuration, however you didn't update the expected output for the test, hence it fails. So the fix would be to update the expected output for the test file and all should be fine :)

@VincentLanglet
Copy link
Contributor Author

Thanks

@SpacePossum
Copy link
Contributor

Might be worth running on latest SF src and see how many hits there are and/or align with SF team, anyways, change look good to me, so I would welcome these :)

@VincentLanglet
Copy link
Contributor Author

Might be worth running on latest SF src and see how many hits there are and/or align with SF team, anyways, change look good to me, so I would welcome these :)

I found 8 occurences of null| and tons of |null

@SpacePossum
Copy link
Contributor

thanks, sounds like a good thing you got here :)

@Wirone
Copy link
Member

Wirone commented Oct 5, 2023

@nicolas-grekas can you confirm we can safely add it? @VincentLanglet did you try to run it against Symfony codebase?

@keradus keradus changed the title feat: Add ordered_types to SymfonySet feat: Add ordered_types to @Symfony Oct 6, 2023
@VincentLanglet VincentLanglet force-pushed the orderTypeSymfony branch 2 times, most recently from 0f74193 to 703107d Compare October 7, 2023 07:28
@keradus
Copy link
Member

keradus commented Oct 9, 2023

@VincentLanglet , no need to poke me for review.
the missing element here is voice of Symfony to declare if they want to extend SymfonySet this way , eg #7356 (comment)

@VincentLanglet
Copy link
Contributor Author

Maybe @OskarStark you could say if it's ok to add this to the Symfony standard or ping someone who could decide

@OskarStark
Copy link
Contributor

I will bring this PR to the core-Team, let's wait a decision.

@VincentLanglet
Copy link
Contributor Author

I will bring this PR to the core-Team, let's wait a decision.

Any news @OskarStark ? ^^'

@keradus
Copy link
Member

keradus commented Jan 15, 2024

vide symfony/symfony#53544

xabbuh added a commit to symfony/symfony that referenced this pull request Jan 16, 2024
…radus)

This PR was squashed before being merged into the 7.1 branch.

Discussion
----------

CS: enable ordered_types.null_adjustment=always_last

| Q             | A
| ------------- | ---
| Branch?       | 7.1
| Bug fix?      | no
| New feature?  | no <!-- please update src/**/CHANGELOG.md files -->
| Deprecations? | no <!-- please update UPGRADE-*.md and src/**/CHANGELOG.md files -->
| Issues        | Fix CS
| License       | MIT

This PR is to apply rule to put `null` as last type variant for type declaration.
Sf ruleset is already having enabled very same rule for types in phpdoc.

Diff is very minimal, most of codebase already follows it.

If merged, we can adjust official ruleset (PR already prepared by community: PHP-CS-Fixer/PHP-CS-Fixer#7356 )

Commits
-------

67a5d2d CS: enable ordered_types.null_adjustment=always_last
Copy link
Member

@keradus keradus left a comment

Choose a reason for hiding this comment

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

Accepted by SF community

@@ -147,6 +147,10 @@ public function getRules(): array
],
'sort_algorithm' => 'alpha',
],
'ordered_types' => [
Copy link
Member

Choose a reason for hiding this comment

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

would be good to cover this set change in test.
ie tests/Fixtures/Integration/set/@Symfony.test

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think It won't be compatible with the fact the test is running in PHP 7.4

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@keradus I don't see how to add this in Symfony.set since it must run in PHP 7.4 too.

But there is already test about this change since I needed to update the following code:
#7356 (comment)

@VincentLanglet
Copy link
Contributor Author

Test are green and I don't think #7356 (comment) is doable until we drop 7.4 support.

Is it something else to do @Wirone @keradus ? :)

@keradus keradus merged commit b6dc318 into PHP-CS-Fixer:master Feb 2, 2024
26 checks passed
danog pushed a commit to zoonru/PHP-CS-Fixer that referenced this pull request Feb 2, 2024
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.

None yet

6 participants