8000 Simplify PHP CS Fixer config by keradus · Pull Request #39772 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

Simplify PHP CS Fixer config #39772

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Jan 14, 2021
Merged

Simplify PHP CS Fixer config #39772

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 2 additions & 7 deletions .php_cs.dist
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,18 +6,13 @@ if (!file_exists(__DIR__.'/src')) {

return PhpCsFixer\Config::create()
->setRules([
'@PHP71Migration' => true,
'@PHPUnit75Migration:risky' => true,
'@Symfony' => true,
'@Symfony:risky' => true,
'@PHPUnit75Migration:risky' => true,
'php_unit_dedicate_assert' => ['target' => '5.6'],
'array_syntax' => ['syntax' => 'short'],
'fopen_flags' => false,
'protected_to_private' => false,
'native_constant_invocation' => true,
Copy link
Member Author
@keradus keradus Jan 10, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

enabling the rules as-is is causing the rule to use strict => false setting (ref https://cs.symfony.com/doc/rules/constant_notation/native_constant_invocation.html#strict )

when rule was introduced, on request of Sf, we set up strict => true for Symfony ruleset.
if you prefer strict => false, we can adjust the ruleset definition

what is your preference?

Copy link
Member

Choose a reason for hiding this comment

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

we now decided to always add the leading \ to all constants

Copy link
Member Author
@keradus keradus Jan 13, 2021 •
edited
L 8000 oading

Choose a reason for hiding this comment

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

ack, thanks for the confirmation

out of curiosity - what was the reason for this change?

Copy link
Member

Choose a reason for hiding this comment

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

measurable perf difference if I recall well

'combine_nested_dirname' => true,
'list_syntax' => ['syntax' => 'short'],
Copy link
Member Author

Choose a reason for hiding this comment

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

FYI we gonna incorporate it in PHP migration ruleset PHP-CS-Fixer/PHP-CS-Fixer#5428

Copy link
Member

Choose a reason for hiding this comment

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

then let's remove this line too, and merge this PR after PHP-CS-Fixer/PHP-CS-Fixer#5428 is merged?

Copy link
Member Author

Choose a reason for hiding this comment

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

we need to extend integration tests to reflect this change. can't promise it's done in few days.
that's why i decided to have PR to cleanup what's currently possible, and do further cleanup in future, when changes on PHP CS Fixer are done and when fabbot is updated

'visibility_required' => ['property', 'method', 'const'],
'ternary_to_null_coalescing' => true,
])
->setRiskyAllowed(true)
->setFinder(
Expand Down
0