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

minor: Follow PSR12 ordered imports in Symfony ruleset #6712

Merged
merged 3 commits into from Dec 17, 2022

Conversation

VincentLanglet
Copy link
Contributor

@VincentLanglet VincentLanglet commented Dec 16, 2022

PSR12 ordered imports is set this way

'ordered_imports' => [

And Symfony extends the PSR12

but then when setting the ordered import, it's true

'ordered_imports' => true,

Which means the previous config is lost, in order to have an alpha sort.

IMHO both config should be used.

  • alpha sort like it always was in Symfony Ruleset
  • sorting classes/functions/constantes to be PSR12

I would say it was never updated when Symfony moved from PSR2 to PSR12.

I'll fix test if this is accepted

@coveralls
Copy link

coveralls commented Dec 16, 2022

Pull Request Test Coverage Report for Build 3720836281

  • 9 of 9 (100.0%) changed or added relevant lines in 2 files are covered.
  • 323 unchanged lines in 36 files lost coverage.
  • Overall coverage increased (+0.4%) to 93.382%

Files with Coverage Reduction New Missed Lines %
src/DocBlock/DocBlock.php 1 90.91%
src/Fixer/FunctionNotation/NativeFunctionInvocationFixer.php 1 97.93%
src/Console/Application.php 2 48.94%
src/Fixer/ArrayNotation/NormalizeIndexBraceFixer.php 2 0%
src/Fixer/CastNotation/NoUnsetCastFixer.php 2 0%
src/Fixer/ClassNotation/ClassAttributesSeparationFixer.php 2 94.51%
src/Fixer/NamespaceNotation/CleanNamespaceFixer.php 2 29.41%
src/Fixer/Phpdoc/PhpdocTagCasingFixer.php 2 82.35%
src/RuleSet/RuleSetInterface.php 2 0%
src/Tokenizer/Transformers.php 2 26.09%
Totals Coverage Status
Change from base Build 3623594219: 0.4%
Covered Lines: 22674
Relevant Lines: 24281

💛 - Coveralls

@keradus
Copy link
Member

keradus commented Dec 16, 2022

I think we should get Sf deciders to make that call here.

@VincentLanglet
Copy link
Contributor Author

I'll ping @nicolas-grekas then

@nicolas-grekas
Copy link

Sorry I'm not sure I fully grasp what this would change. But yes, we list "use" in alpha order. class, function then const LGTM also.

@VincentLanglet
Copy link
Contributor Author

Sorry I'm not sure I fully grasp what this would change. But yes, we list "use" in alpha order. class, function then const LGTM also.

Cf https://github.com/PHP-CS-Fixer/PHP-CS-Fixer/blob/master/doc/rules/import/ordered_imports.rst#rule-sets, Symfony use ordered_imports with default config, which is

['sort_algorithm' => 'alpha']

so it's loosing the PSR12 part

'imports_order' => ['class', 'function', 'const']

Which means that even if Symfony surely wants to respect the PSR12, the SymfonyRuleset doesn't fix this automatically.

When Symfony override the rule to set the sort algorithm to alpha, we need to repeat the import order too.

@keradus
Copy link
Member

keradus commented Dec 16, 2022

looks like there is an alignment ;)

@VincentLanglet
Copy link
Contributor Author

I fixed the tests @keradus

Btw, all ruleset will use at least 'imports_order' => ['class', 'function', 'const'] now.

It might be interesting to change the default value of the rule from

['sort_algorithm' => 'alpha']

to

['imports_order' => ['class', 'function', 'const'], 'sort_algorithm' => 'alpha']

in next major then. Does it require a comment somewhere ?

@julienfalque
Copy link
Member

Does it require a comment somewhere ?

We use to add TODOs for that, e.g. // @TODO set to ['class', 'function', 'const'] on 4.0 next to the setDefault() call in OrderedImportsFixer.

@VincentLanglet
Copy link
Contributor Author

Thanks @julienfalque, it's done

@julienfalque julienfalque changed the title [RFC] Follow PSR12 ordered imports in Symfony ruleset minor: Follow PSR12 ordered imports in Symfony ruleset Dec 17, 2022
@julienfalque julienfalque merged commit fc7dd53 into PHP-CS-Fixer:master Dec 17, 2022
@julienfalque
Copy link
Member

Thank you @VincentLanglet.

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.

None yet

5 participants