-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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 single_line_empty_body
config only_for_constructors
#7962
Draft
rudiedirkx
wants to merge
3
commits into
PHP-CS-Fixer:master
Choose a base branch
from
rudiedirkx:single_line_empty_body-only_for_constructors
base: master
Could not load branches
Branch not found: {{ refName }}
Could not load tags
Nothing to show
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Draft
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -15,24 +15,45 @@ | |
namespace PhpCsFixer\Fixer\Basic; | ||
|
||
use PhpCsFixer\AbstractFixer; | ||
use PhpCsFixer\Fixer\ConfigurableFixerInterface; | ||
use PhpCsFixer\FixerConfiguration\FixerConfigurationResolver; | ||
use PhpCsFixer\FixerConfiguration\FixerConfigurationResolverInterface; | ||
use PhpCsFixer\FixerConfiguration\FixerOptionBuilder; | ||
use PhpCsFixer\FixerDefinition\CodeSample; | ||
use PhpCsFixer\FixerDefinition\FixerDefinition; | ||
use PhpCsFixer\FixerDefinition\FixerDefinitionInterface; | ||
use PhpCsFixer\Tokenizer\Token; | ||
use PhpCsFixer\Tokenizer\Tokens; | ||
|
||
final class SingleLineEmptyBodyFixer extends AbstractFixer | ||
final class SingleLineEmptyBodyFixer extends AbstractFixer implements ConfigurableFixerInterface | ||
{ | ||
public function getDefinition(): FixerDefinitionInterface | ||
{ | ||
return new FixerDefinition( | ||
'Empty body of class, interface, trait, enum or function must be abbreviated as `{}` and placed on the same line as the previous symbol, separated by a single space.', | ||
[new CodeSample('<?php function foo( | ||
[ | ||
new CodeSample('<?php function foo( | ||
int $x | ||
) | ||
{ | ||
} | ||
')], | ||
' | ||
), | ||
new CodeSample('<?php | ||
class Foo | ||
{ | ||
public function __construct() | ||
{ | ||
} | ||
|
||
public function foo() | ||
{ | ||
} | ||
} | ||
', | ||
['only_for_constructors' => true] | ||
Check warning on line 54 in src/Fixer/Basic/SingleLineEmptyBodyFixer.php GitHub Actions / PHP 8.3 mutation tests
Check warning on line 54 in src/Fixer/Basic/SingleLineEmptyBodyFixer.php GitHub Actions / PHP 8.3 mutation tests
|
||
) | ||
], | ||
); | ||
} | ||
|
||
|
@@ -46,6 +67,16 @@ | |
return -19; | ||
} | ||
|
||
protected function createConfigurationDefinition(): FixerConfigurationResolverInterface | ||
{ | ||
return new FixerConfigurationResolver([ | ||
(new FixerOptionBuilder('only_for_constructors', 'Whether to ONLY fold __construct() methods, nothing else.')) | ||
->setAllowedTypes(['bool']) | ||
Check warning on line 74 in src/Fixer/Basic/SingleLineEmptyBodyFixer.php GitHub Actions / PHP 8.3 mutation tests
|
||
->setDefault(false) | ||
->getOption(), | ||
]); | ||
} | ||
|
||
public function isCandidate(Tokens $tokens): bool | ||
{ | ||
if (\defined('T_ENUM') && $tokens->isTokenKindFound(T_ENUM)) { // @TODO: drop condition when PHP 8.1+ is required | ||
|
@@ -62,6 +93,13 @@ | |
continue; | ||
} | ||
|
||
if (true === $this->configuration['only_for_constructors']) { | ||
$funcNameIndex = $tokens->getNextNonWhitespace($index); | ||
if ('__construct' !== $tokens[$funcNameIndex]->getContent()) { | ||
Check warning on line 98 in src/Fixer/Basic/SingleLineEmptyBodyFixer.php GitHub Actions / PHP 8.3 mutation tests
|
||
continue; | ||
} | ||
} | ||
|
||
$openBraceIndex = $tokens->getNextTokenOfKind($index, ['{', ';']); | ||
if (!$tokens[$openBraceIndex]->equals('{')) { | ||
continue; | ||
|
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi and thanks for your request.
Is this part of a standard or used by a bigger (F)OSS community? For us that is a condition to add a rule to this project. Otherwise, you can create a custom fixer and use that in your projects. Thanks for understanding.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Personally I don't mind if this is part of a bigger community's standard. I just don't like the approach. IMHO it should be an option for configuring places where
{}
should be applied (constructor, method signature, loop, try/catch etc), by default all of them should be enabled, with ability to narrow the scope. This particular option is not flexible enough.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm challenging the need for proposed option as well as for generic option.
I do not want to have option for sth because "it is technically possible", but because it's a recommended standard.
Until proven differently (with link to example community that will benefit from given configuration), I'm against config for this rule.
Having one approach for given element (eg constructor), but another approach for another element - it's not a standard at all.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not a standard as far as I know. It's just how I like it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@keradus it was discussed in #7809, it really looks like making it configurable is something nice to have.