Php-cs-fixer: no_extra_consecutive_blank_lines - rename to no_extra_blank_lines

Created on 1 Jul 2016 · 13Comments · Source: FriendsOfPHP/PHP-CS-Fixer

A little example running this fixer in symfony/symfony:2.7:

$ php-cs-fixer fix . -vvv --rules=no_useless_else
$ git diff upstream/2.7 | cat -A
diff --git a/src/Symfony/Bridge/Twig/NodeVisitor/TranslationDefaultDomainNodeVisitor.php b/src/Symfony/Bridge/Twig/NodeVisitor/TranslationDefaultDomainNodeVisitor.php$
index c923df9..691aceb 100644$
--- a/src/Symfony/Bridge/Twig/NodeVisitor/TranslationDefaultDomainNodeVisitor.php$
+++ b/src/Symfony/Bridge/Twig/NodeVisitor/TranslationDefaultDomainNodeVisitor.php$
@@ -48,13 +48,13 @@ protected function doEnterNode(\Twig_Node $node, \Twig_Environment $env)$
                 $this->scope->set('domain', $node->getNode('expr'));$
 $
                 return $node;$
-            } else {$
+            }  $
                 $var = $env->getParser()->getVarName();$
                 $name = new \Twig_Node_Expression_AssignName($var, $node->getLine());$
                 $this->scope->set('domain', new \Twig_Node_Expression_Name($var, $node->getLine()));$
 $
                 return new \Twig_Node_Set(false, new \Twig_Node(array($name)), new \Twig_Node(array($node->getNode('expr'))), $node->getLine());$
-            }$
+            $
         }$
 $
         if (!$this->scope->has('domain')) {$

kinrefactoring

Source

phansys

👍1

All 13 comments

It doesn't generate anything, the fixer only removes stuff.
For trailing space there are fixers like trailing_spaces whitespacy_lines etc. and we've tests to make sure the useless_else fixer runs before those.
The indenting should be fixed by some other fixer we may or may not have.
Anyway, each fixer should have one task, otherwise we end up with N-fixers being able to fix indentation issues.
IMHO this issue is not valid.

(btw. SF doesn't want there useless else statements removed; https://github.com/symfony/symfony/pull/17828)

SpacePossum on 1 Jul 2016

👍1

I get your point @SpacePossum. Then, just thinking, we really should remove extra line breaks between properties / constants declarations at #1889?

phansys on 1 Jul 2016

Not sure about your point of 1889, it tries to stick to the current CS of a class when it splits up the declarations right? I think that is the best we can do. If there is a rule for (not) having empty lines between the properties/constants we could make a fixer for that and run it after the one in 1889 :)

SpacePossum on 2 Jul 2016

Right @SpacePossum, I mean that in the same fixer we are currently following these steps:

Exploding combined property / const declarations in different ones, using as separator between them the original separators we found at the combined declaration.
Checking if any of these separators are line breaks and replacing them with just one line break

Note that point 2 isn't part of the main goal of the fixer, so I think that transformation should go in another (existent or new) fixer.

phansys on 3 Jul 2016

I see your point :)
The catch is, I think, that when there is one line having multiple statements it is impossible to know if it was intended to be separated with one empty line between each line break.
For example:

class Foo 
{
    public function bar()
    {
    }

    private $a, $b;

    public function bar()
    {
    }
}

I would expect this to be fixed to:

class Foo 
{
    public function bar()
    {
    }

    private $a;
    private $b;

    public function bar1()
    {
    }
}

because now there is one statement for each line and the indentation is preserved.
This also means if the indentation is not PSR (i.e. tabs or non-four-spaces etc.) the new indentation is not PSR as well. I would consider this a feature as one might want to use your fixer to split up the statements, but doesn't want the indentation to fixed.

(Same goes for Windows line breaks etc., but we have to be pragmatic here, fix as good as we can without adding a lot of overhead supporting cases no one is prop. doing.)

The most important thing is to; touch as little as possible, try to fix to best effort considering complexity and performance of the fixer, make sure not to create conflicts with other fixers.
I think your fixer matches that, so I wouldn't change for now.

You could open a ticket (or PR) for the no_extra_consecutive_blank_lines fixer on master to add a configuration option for between class statements (or something like that) and create a counter part fixer to add the empty lines if wanted. Those should run after your fixer.

SpacePossum on 3 Jul 2016

Thank you for the detailed feedback @SpacePossum. I agree with all your thoughts, except the last one: no_extra_consecutive_blank_lines is for consecutive blank lines; so, the quantity of blank lines must be at least 2 in that case. IMO, here we are talking about "no extra consecutive line breaks" (or something similar).

phansys on 3 Jul 2016

Renaming the last fixer might be a good idea. I've added a bunch of configuration options that are indeed not only dealing with consecutive empty lines but with single lines as well. Good point :)

SpacePossum on 3 Jul 2016

👍1

could anyone send a PR for that?