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

Conversation

rudiedirkx
Copy link

Don't fold anything, except a class' __construct(), because that looks better with PHP 8's property promotion.

I have plenty of empty methods that are overridden in children, or that are overrides of parents. I don't want to fold those, because that looks weird:

protected function applyConfig(int $priority, array $config, TreeAwareConfigContainer $parent): void {}

(where are the brackets?? am I in an interface?? help!!)

But empty PHP 8 constructors look weird if they're NOT folded:

public function __construct(
	protected string $name,
)
{

}

or

public function __construct(
	protected string $name,
) {
}

etc.


If the general idea is acceptable, it could be configured differently, with a context config option or something. Default is everything:

[
	'contexts' => ['class', 'interface', 'trait', 'enum', 'method', 'constructor', 'function'], // more?
],

@rudiedirkx rudiedirkx changed the title single_line_empty_body config only_for_constructors feat: Add single_line_empty_body config only_for_constructors Apr 22, 2024
@coveralls
Copy link

Coverage Status

coverage: 96.055% (+0.002%) from 96.053%
when pulling 2763af9 on rudiedirkx:single_line_empty_body-only_for_constructors
into 5c6b9a4 on PHP-CS-Fixer:master.

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.

@Wirone Wirone marked this pull request as draft April 29, 2024 07:18
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

4 participants