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 single_line_empty_body config only_for_constructors #7962

Draft
wants to merge 3 commits into
base: master
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
36 changes: 36 additions & 0 deletions doc/rules/basic/single_line_empty_body.rst
Original file line number Diff line number Diff line change
Expand Up @@ -6,12 +6,26 @@ 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.

Configuration
-------------

``only_for_constructors``
Copy link
Member

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.

Copy link
Member

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.

Copy link
Member

@keradus keradus Apr 22, 2024

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.

Copy link
Author

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.

Copy link
Member

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.

~~~~~~~~~~~~~~~~~~~~~~~~~

Whether to ONLY fold __construct() methods, nothing else.

Allowed types: ``bool``

Default value: ``false``

Examples
--------

Example #1
~~~~~~~~~~

*Default* configuration.

.. code-block:: diff

--- Original
Expand All @@ -23,6 +37,28 @@ Example #1
-}
+) {}

Example #2
~~~~~~~~~~

With configuration: ``['only_for_constructors' => true]``.

.. code-block:: diff

--- Original
+++ New
<?php
class Foo
{
- public function __construct()
- {
- }
+ public function __construct() {}

public function foo()
{
}
}

Rule sets
---------

Expand Down
44 changes: 41 additions & 3 deletions src/Fixer/Basic/SingleLineEmptyBodyFixer.php
Original file line number Diff line number Diff line change
Expand Up @@ -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

View workflow job for this annotation

GitHub Actions / PHP 8.3 mutation tests

Escaped Mutant for Mutator "ArrayItemRemoval": --- Original +++ New @@ @@ { } } -', ['only_for_constructors' => true])]); +', [])]); } /** * {@inheritdoc}

Check warning on line 54 in src/Fixer/Basic/SingleLineEmptyBodyFixer.php

View workflow job for this annotation

GitHub Actions / PHP 8.3 mutation tests

Escaped Mutant for Mutator "TrueValue": --- Original +++ New @@ @@ { } } -', ['only_for_constructors' => true])]); +', ['only_for_constructors' => false])]); } /** * {@inheritdoc}
)
],
);
}

Expand All @@ -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

View workflow job for this annotation

GitHub Actions / PHP 8.3 mutation tests

Escaped Mutant for Mutator "ArrayItemRemoval": --- Original +++ New @@ @@ } protected function createConfigurationDefinition() : FixerConfigurationResolverInterface { - return new FixerConfigurationResolver([(new FixerOptionBuilder('only_for_constructors', 'Whether to ONLY fold __construct() methods, nothing else.'))->setAllowedTypes(['bool'])->setDefault(false)->getOption()]); + return new FixerConfigurationResolver([(new FixerOptionBuilder('only_for_constructors', 'Whether to ONLY fold __construct() methods, nothing else.'))->setAllowedTypes([])->setDefault(false)->getOption()]); } public function isCandidate(Tokens $tokens) : bool {
->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
Expand All @@ -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

View workflow job for this annotation

GitHub Actions / PHP 8.3 mutation tests

Escaped Mutant for Mutator "NotIdentical": --- Original +++ New @@ @@ } if (true === $this->configuration['only_for_constructors']) { $funcNameIndex = $tokens->getNextNonWhitespace($index); - if ('__construct' !== $tokens[$funcNameIndex]->getContent()) { + if ('__construct' === $tokens[$funcNameIndex]->getContent()) { continue; } }
continue;
}
}

$openBraceIndex = $tokens->getNextTokenOfKind($index, ['{', ';']);
if (!$tokens[$openBraceIndex]->equals('{')) {
continue;
Expand Down