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

Proposal: enable single_line_empty_body rule #14

Open
juliushaertl opened this issue Aug 24, 2023 · 5 comments
Open

Proposal: enable single_line_empty_body rule #14

juliushaertl opened this issue Aug 24, 2023 · 5 comments

Comments

@juliushaertl
Copy link
Member

juliushaertl commented Aug 24, 2023

Using more and more constructor property promotion with PHP 8.0 now, I more often end up having empty contructor bodies and the current ruleset requires to always have the opening and closing curly braces in new lines.

Might be personal preference which just triggers me a lot, but my suggestion would be to switch on the single_line_empty_body rule to allow for a more compact way of writing this

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.

https://cs.symfony.com/doc/rules/basic/single_line_empty_body.html

Before

class MyService {
	public function __construct(
		private IURLGenerator $urlGenerator,
	) {
	}
}

After

class MyService {
	public function __construct(
		private IURLGenerator $urlGenerator,
	) {}
}
@juliushaertl
Copy link
Member Author

What do you think @ChristophWurst @nickvergessen @come-nc to pick some people ;)

@nickvergessen
Copy link
Member

Yeah, doing that one wrong very often as well, also in all the stub-files

I'd be okay.
Can we allow both patterns? Or would you break/disallow {\n} ?

@juliushaertl
Copy link
Member Author

I couldn't find a way to allow both but prefer this one.

@come-nc
Copy link
Contributor

come-nc commented Aug 28, 2023

I’m used to the "Before" state and I do that on purpose. I like having the closing tag always on its own line and not having to change that when removing the last line in a constructor.

It’s all about clean diff for me, I like when diffs look like:

diff --git a/lib/private/Security/Bruteforce/Backend/DatabaseBackend.php b/lib/private/Security/Bruteforce/Backend/DatabaseBackend.php
index 04f2a7b6397..8d6726bea6e 100644
--- a/lib/private/Security/Bruteforce/Backend/DatabaseBackend.php
+++ b/lib/private/Security/Bruteforce/Backend/DatabaseBackend.php
@@ -33,6 +33,7 @@ class DatabaseBackend implements IBackend {
        public function __construct(
                private IDBConnection $db,
        ) {
+               $this->thing = "init";
        }
 
        /**

And this is less clean:

diff --git a/lib/private/Security/Bruteforce/Backend/DatabaseBackend.php b/lib/private/Security/Bruteforce/Backend/DatabaseBackend.php
index 1304a36af47..8d6726bea6e 100644
--- a/lib/private/Security/Bruteforce/Backend/DatabaseBackend.php
+++ b/lib/private/Security/Bruteforce/Backend/DatabaseBackend.php
@@ -32,7 +32,9 @@ class DatabaseBackend implements IBackend {
 
        public function __construct(
                private IDBConnection $db,
-       ) {}
+       ) {
+               $this->thing = "init";
+       }
 
        /**
         * {@inheritDoc}

What I would be more interested in is PHP-CS-Fixer/PHP-CS-Fixer#6325 , for the same reason of having a clean diff and a clean git blame as well.

All that said, I’m not a strong no on this, if it pleases people and is handled by the cs-fixer I will change habits, I have the pre-commit tool setup anyway.

(Also we’ll need nextcloud/server#39271 first before doing other changes to codestyle config)

@ChristophWurst
Copy link
Member

I'm fine with either. Single line makes the code more compact but I also get @come-nc's idea of clean diffs for removed/added constructor statements.

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

No branches or pull requests

4 participants