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

Require empty line after class definition and before it end #3653

Closed
erickskrauch opened this issue Mar 29, 2018 · 36 comments
Closed

Require empty line after class definition and before it end #3653

erickskrauch opened this issue Mar 29, 2018 · 36 comments

Comments

@erickskrauch
Copy link
Contributor

The PHP version you are using ($ php -v):

PHP 7.1.8

PHP CS Fixer version you are using ($ php-cs-fixer -V):

PHP CS Fixer 2.11.1

The command you use to run PHP CS Fixer:

php-cs-fixer --diff --dry-run --stop-on-violation --using-cache=no -v fix

The configuration file you are using, if any:

<?php
$finder = Symfony\Component\Finder\Finder::create()
    ->in(['api', 'common', 'console'])
    ->notPath('*/runtime')
    ->name('*.php')
    ->ignoreDotFiles(true)
    ->ignoreVCS(true);

return PhpCsFixer\Config::create()
    ->setRules([
        '@PSR2' => true,
        'braces' => [
            'allow_single_line_closure' => false,
            'position_after_functions_and_oop_constructs' => 'same',
        ],
        'blank_line_before_statement' => true,
        'cast_spaces' => [
            'space' => 'none',
        ],
        'linebreak_after_opening_tag' => true,
        'no_whitespace_in_blank_line' => true,
        'non_printable_character' => [
            'use_escape_sequences_in_strings' => true,
        ],
        'ordered_imports' => true,
        'single_quote' => true,
        'whitespace_after_comma_in_array' => true,
        'trailing_comma_in_multiline_array' => true,
    ])
    ->setRiskyAllowed(true)
    ->setFinder($finder);

If applicable, please provide minimum samples of PHP code (as plain text, not screenshots):

  • before running PHP CS Fixer (no changes):
<?php
namespace common\rbac;

final class Roles {

    public const ACCOUNTS_WEB_USER = 'accounts_web_user';

}
  • with unexpected changes applied when running PHP CS Fixer:
   1) project-name/common/rbac/Roles.php (class_definition, braces)
      ---------- begin diff ----------
--- Original
+++ New
@@ @@
 <?php
 namespace common\rbac;
 
 final class Roles {
-
     public const ACCOUNTS_WEB_USER = 'accounts_web_user';
-
 }
 

      ----------- end diff -----------
  • with the changes you expected instead:
No changes

I want to have one blank line after defining the class and before closing it:

class Test {

    // Any contents here

}

But braces and class_definition fixers do not allow to configure it. How can I suppress or configure this param?

@keradus
Copy link
Member

keradus commented Mar 29, 2018

I don't recall that we have such an option.

hm... could you please provide some details how you end up with given coding standard? is it somehow established in some community ?

@erickskrauch
Copy link
Contributor Author

erickskrauch commented Mar 29, 2018

is it somehow established in some community?

Unfortunately, I tend to think that no. This code style is used only in my own projects. Currently, I use a PHPStorm feature to fix code style:

I want to have some solution, that I may include into our CI. For now, I only see a possibility to hard fork braces and class_definition fixers and describe to them this strange code style rules.

@erickskrauch
Copy link
Contributor Author

erickskrauch commented Mar 30, 2018

@keradus, @juliendufresne I want to try to implement this and send the pull request. Could you give me recommendations for implementation? I'm not quite sure how to split responsibilities now. Should I create a separate fixer for this rule or implement it as a configuration parameter of the existing one? In addition, as I understand it, I'll need to explain braces and class_definition fixers that they should ignore empty lines at the beginning and at the end of the class because it will become a responsibility of a separate fixer.

@keradus
Copy link
Member

keradus commented Apr 15, 2018

sorry for late feedback, I was having a little break in real life.

@keradus
Copy link
Member

keradus commented Apr 15, 2018

For us it is important that fixers/rules/configuration either follow a standard like PSR (as we like to promote the usage/following of standards) or are widely used (as we are more likely to get support on the fixer when maintaining it).
I must admit that I do not only not follow it, but I didn't saw it in other repos as well.
Is this a widely followed standard, or just your own personal preference? Could you share some references, who is using it ?

@erickskrauch
Copy link
Contributor Author

erickskrauch commented Apr 15, 2018

As I said before, this is not widely followed standard and it's mostly personal (our team) preference. I saw such code style in some repositories, that follows braces['position_after_functions_and_oop_constructs'] = 'same' to keep same whitespace around properties/methods. But right now I can't provide you any actual reference, except our own code.

But I understand your feelings about maintenance. If you think it's not usable fixer for others than we can just keep it in our custom set of fixers.

@keradus
Copy link
Member

keradus commented Apr 15, 2018

adding it in generic way would be tricky and I will leave decision to you ;)

First of all, we don't like to have rules that can do the same, even partially.
2nd, we don't like to have rules that do the reverse, causing conflicts (first run of PHP CS Fixer does the change with fixer A, 2nd run reverts those changes with fixer B).

For that, here we would need to determine what would be those fixers.
I don't see conflicts with braces nor class_definition. If I'm wrong on that, please prove it ;)

What is see is at least conflict with no_blank_lines_after_class_opening.
Your proposal is good in such way that it allow to configure how many of line can be there.

What I mainly see that to be done is to confirm which existing rule is conflicting, and then decide one by one what to do with them.

Then, for conflict with no_blank_lines_after_class_opening, IMHO we shall deprecate it in favor of new rule.
To to this, it shall become a proxy fixer, that would use new fixer with some concrete configuration, with which the functionality would remain the same.

@erickskrauch
Copy link
Contributor Author

I don't see conflicts with braces nor class_definition. If I'm wrong on that, please prove it ;)

This issue began just with these fixers. I will check it.

// 10 mins later

I performed some tests and found, that ClassDefinitionFixer is removes my dearest empty lines. But for some reasons my fixer implementation won't work if have a priority higher then -25, which equals to BracesFixer.

Then, for conflict with no_blank_lines_after_class_opening, IMHO we shall deprecate it in favor of new rule.

Sure. I can deprecate it and rewrite as proxy to my fixer in PR.

@erickskrauch
Copy link
Contributor Author

I think it's okay to have such behavior. ClassDefinitionFixer normalizes class definition according to widely used style and then my fixer (if enabled) will adjust it for some preferred amount of empty lines.

@keradus
Copy link
Member

keradus commented Apr 15, 2018

I performed some tests and found, that ClassDefinitionFixer is removes my dearest empty lines.

show concrete examples

@erickskrauch
Copy link
Contributor Author

Another test. This time I've setted up a clean environment. Input file:

<?php
class Test {

    public function method() {
        // body
    }

}

Initial config:

<?php
$finder = PhpCsFixer\Finder::create()
    ->in(__DIR__);

return \PhpCsFixer\Config::create()
    ->setRules([
        'braces' => [
            'position_after_functions_and_oop_constructs' => 'same',
        ],
        'class_definition' => true,
        'class_attributes_separation' => true,
    ])
    ->setFinder($finder);

Result:

 <?php
 class Test {
-
     public function method() {
         // body
     }
-
 }

Then I got the same result for:

  • ONLY braces.
  • ONLY class_attributes_separation.

ONLY class_definition produced such output:

 <?php
-class Test {
+class Test
+{
 
     public function method() {
         // body
     }
 
 }

Looks like it's partially duplicating braces behavior about position_after_functions_and_oop_constructs.


At least, now I have an answer why my fixer must have less priority than BracesFixer.

@keradus
Copy link
Member

keradus commented Apr 17, 2018

that's the thing.
if "by design" new fixer gonna do the same as any other fixer, it won't be accepted, as they can be configured to follow different styleguide, thus those rules will be in conflict.
for that, if rule will do sth that other rule already does, but in more generic way, allows for config and so, it shall deprecate that part of already existing rule

@erickskrauch
Copy link
Contributor Author

So, this is my suggestion:

  • Add to the fixer next config params:

    • count_lines_before_class_body = 1;
    • count_lines_after_class_body = 1;
    • count_lines_before_anonymous_class_body = 1;
    • count_lines_after_anonymous_class_body = 1;
  • Replace no_blank_lines_after_class_opening with a proxy to this fixer with all params set to zeroes.

If you agreed, then I'll rework my PR to complete this features.

P.S. maybe I should change some names?

@erickskrauch
Copy link
Contributor Author

@keradus

@InvisibleSmiley
Copy link

I was just about to create a bug report for the behavior of the braces and class_attributes_separation rules as well as class_definition (like erickskrauch commented on 17 Apr). Those are not meant to enforce the removal of blank lines but currently they do.

The above mentioned rules must not dictate the amount of blank lines. They should be indifferent to whether there is one or not, be it at the beginning or end of a block. I can file a bug report for that if need be.

Otherwise I'd love to see this particular issue to be fixed, i.e. have the ability to require exactly one blank line at the start and end of a class body.

Personally I think it'd be best to first fix the existing rules and only then create a new one, but YMMV. Please let me know if I you'd like me to separate the two issues.

@erickskrauch
Copy link
Contributor Author

@InvisibleSmiley I'm agreed with you. But I still have no response from the core team to have any solution about final implementation, that will satisfy everyone.

@esemlabel
Copy link

Currently there is no ability to add blank lines before/after class body, if position_after_functions_and_oop_constructs = same. Need fix.

@erickskrauch
Copy link
Contributor Author

@esemlabel, you may try to use our custom implementation: https://github.com/elyby/php-code-style#elyblank_line_around_class_body

@esemlabel
Copy link

@erickskrauch, not my case, I'm using vscode extension.

@erickskrauch
Copy link
Contributor Author

@esemlabel, but this extension can use the configuration from .php_cs file, so there is no difference between usage of PHP-CS-Fixer directly or via VSCode extension.

@esemlabel
Copy link

@erickskrauch, I'd like to have all clear and in one place, instead of installing additional fixes and remember to uninstall after php cs fixer updates.
Will wait..

@erickskrauch
Copy link
Contributor Author

I'm waiting for any response for more than a year. Good luck :)

@smuuf
Copy link

smuuf commented Mar 15, 2020

This would be great.

@SpacePossum
Copy link
Contributor

As this is not part of a standard or widely used in (F)OSS I prefer not that have this in the project to avoid the maintenance cost of the more complex code. IMHO this would be better fitted in a 3rd party custom fixer.

@erickskrauch
Copy link
Contributor Author

Okay 😢

@dpi
Copy link
Contributor

dpi commented May 11, 2022

Drupal is an example with this standard requirement: https://www.drupal.org/node/608152#indenting , with class style:

class Myclass {

  use MyTrait;

  public function foo(): void {
    $hello = $world;
  }

}

@RobinHoutevelts
Copy link

RobinHoutevelts commented Aug 9, 2022

(Coming from Drupal)

Has anyone found a rule for this? I don't mind it being a third party package.

Edit: Found #3688 (comment) but seems that the linked ely/php-code-style package is still pinned to v2 of php-cs-fixer.

I'll move my focus there. Created issue elyby/php-code-style#11

RobinHoutevelts added a commit to wieni/wmcodestyle that referenced this issue Aug 9, 2022
See PHP-CS-Fixer/PHP-CS-Fixer#3653 (comment)
See elyby/php-code-style#11

---

It's also a Drupal Coding Standards requirement.

Added because Wienis with Drupal Coding Standards experience
got OCD when looking at our code!
RobinHoutevelts added a commit to wieni/wmcodestyle that referenced this issue Oct 10, 2022
See PHP-CS-Fixer/PHP-CS-Fixer#3653 (comment)
See elyby/php-code-style#11

---

It's also a Drupal Coding Standards requirement.

Added because Wienis with Drupal Coding Standards experience
got OCD when looking at our code!
@FlorentTorregrosa
Copy link

Hi,

Coming from Drupal too. Upgrading from Drupal 9 to Drupal 10 resulted in updating friendsofphp/php-cs-fixer from 3.4.0 to 3.13.1.

And now the blank line after the class definition is removed but not the one at the end.

I tried to add https://github.com/elyby/php-code-style#elyblank_line_around_class_body but then I have 2 lines at the end.

Here is my config: https://gitlab.com/florenttorregrosa-drupal/docker-drupal-project/-/blob/10.x/scripts/quality/phpcs_fixer/.php-cs-fixer.dist.php

I have checked that no_blank_lines_after_class_opening was set to FALSE.

I can't figure out which rule is removing the blank line after class opening and the one adding the line at the end.

Thanks for any help.

@keradus
Copy link
Member

keradus commented Dec 28, 2022

fix -vvv file.php will list you all rules that were applied for given file, @FlorentTorregrosa

@FlorentTorregrosa
Copy link

@keradus thanks a lot!

~/sites/docker-drupal-project10$ make quality-fix-php-cs-fixer 
PHP CS Fixer 3.13.1 Oliva by Fabien Potencier and Dariusz Ruminski.
PHP runtime: 8.1.7
Loaded config drupal8 from "./scripts/quality/phpcs_fixer/.php-cs-fixer.dist.php".
Using cache file "/tmp/.php_cs.cache".
Paths from configuration file have been overridden by paths provided as command arguments.
F                                                                                                                                                                                                      1 / 1 (100%)
Legend: .-no changes, F-fixed, S-skipped (cached or empty file), I-invalid file syntax (file ignored), E-error
   1) /project/app/modules/custom/myproject_core/tests/src/Unit/Controller/MyProjectControllerTest.php (class_definition, braces, Drupal/blank_line_before_end_of_class)

Fixed 1 of 1 files in 0.020 seconds, 20.000 MB memory used

Detected deprecations in use:
- Option "tokens: use_trait" used in `no_extra_blank_lines` rule is deprecated, use the rule `class_attributes_separation` with `elements: trait_import` instead.
- Rule "no_trailing_comma_in_list_call" is deprecated. Use "no_trailing_comma_in_singleline" instead.
- Rule "no_trailing_comma_in_singleline_array" is deprecated. Use "no_trailing_comma_in_singleline" instead.

I think this would save my life in the future too.

Ok, I think I will see the options of native rules and maybe reach out https://github.com/drupol/phpcsfixer-configs-drupal, if it provides a Drupal/blank_line_before_end_of_class, maybe it will also be able to provide a Drupal/blank_line_after_start_of_class.

@FlorentTorregrosa
Copy link

So this behavior is coming from braces, but it is not manageable with the currently available options. so I will check with additionnal fixers.

@keradus
Copy link
Member

keradus commented Dec 29, 2022

glad to help!

also please be mindful that we are deprecating the braces fixer [in favour of few other fixers], don't push much effort to make braces work for your case, imho.

@FlorentTorregrosa
Copy link

Ok, thanks, an additional argument for me to see in "contrib" fixers.

I opened discussions on drupol/phpcsfixer-configs-drupal#7

@sovetski
Copy link

Hello, any news about this subject?

@keradus
Copy link
Member

keradus commented Jan 25, 2023

with this news: #3653 (comment)
nobody on the core repo working on it.

If you want the decision to be reevaluated, please raise new details about how widely adopted that style is and which communities would benefit from such feature, as well as your rediness to work on it.

@zeleniy
Copy link

zeleniy commented Jun 17, 2023

Eclipse PDT has about same set of preferences as well:
Screen Shot 2023-06-17 at 13 25 18

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests