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

Trailing Space in a comment before the code to be fixed adds unnecessary empty line #3457

Closed
dbaltas opened this issue Jan 19, 2018 · 7 comments

Comments

@dbaltas
Copy link

dbaltas commented Jan 19, 2018

Having some code with a potential fix and a comment with a trailing space right before the code, results to an empty line being added in the fix.
If the comment has no trailing space, fixer works as expected.

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

=> PHP 7.1.8

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

=> 2.10.0

The command you use to run PHP CS Fixer:

=> php-cs-fixer fix a.php

Sample PHP code (a.php):

  • before running PHP CS Fixer (no changes):
<?php

function foo() : void
{
    // there is a trailing space in this comment! 
    $var1 = (
        $var2 ?? 0);
}
  • with unexpected changes applied when running PHP CS Fixer:
<?php

function foo() : void
{
    // there is a trailing space in this comment!
    $var1 = (
        $var2 ?? 0
 
    );
}

In the resulted code above, there is an empty line between $var2 ?? 0 and the closing parenthesis

  • with the changes you expected instead:
<?php

function foo() : void
{
    // there is a trailing space in this comment!
    $var1 = (
        $var2 ?? 0
    );
}
@SpacePossum
Copy link
Contributor

Hi and thanks for reporting,

Could you share your configuration? Using the default configuration of this project on v.2.10.0 does not show the issue so it is hard to track down without knowing which rules are applied, thanks!

@SpacePossum SpacePossum added the status/input required For the issue to be confirmed or the PR to be process; additional input of the author is required label Jan 19, 2018
dbaltas added a commit to dbaltas/PHP-CS-Fixer that referenced this issue Jan 20, 2018
@dbaltas
Copy link
Author

dbaltas commented Jan 20, 2018

Here is a test reproducing the issue dbaltas@b4ef7ea
even though it breaks the ./check_trailing_spaces.sh script!

@keradus
Copy link
Member

keradus commented Jan 20, 2018

maybe just raise a PR with it ?

@dbaltas
Copy link
Author

dbaltas commented Jan 21, 2018

Well, I didn't raise a PR because I am not close to suggest a fix.

@SpacePossum SpacePossum removed the status/input required For the issue to be confirmed or the PR to be process; additional input of the author is required label Jan 23, 2018
dbaltas added a commit to dbaltas/PHP-CS-Fixer that referenced this issue Jan 28, 2018
dbaltas added a commit to dbaltas/PHP-CS-Fixer that referenced this issue Jan 28, 2018
dbaltas added a commit to dbaltas/PHP-CS-Fixer that referenced this issue Jan 28, 2018
dbaltas added a commit to dbaltas/PHP-CS-Fixer that referenced this issue Jan 28, 2018
… fix when previous line has a trailing whitespace
dbaltas added a commit to dbaltas/PHP-CS-Fixer that referenced this issue Jan 28, 2018
… fix when previous line has a trailing whitespace
dbaltas added a commit to dbaltas/PHP-CS-Fixer that referenced this issue Jan 28, 2018
… fix when previous line has a trailing whitespace
@SpacePossum
Copy link
Contributor

I retried this on 2.2, 2.10 and master and cannot reproduce this issue anymore.
@dbaltas my check might not be all complete, maybe I didn't setup the failing cases correctly so please; if you've the time please retry with any of the latest stable released versions (2.*) of the fixer and let me know if this is still an issue, if so I reopen it :) thanks for reporting!

@dbaltas
Copy link
Author

dbaltas commented Jun 22, 2018

The issue persists in 2.12.1

steps to reproduce

rm -rf /tmp/cs-fix
mkdir /tmp/cs-fix
cd /tmp/cs-fix
curl https://gist.githubusercontent.com/dbaltas/18befe7f6582b10168557a0fcdcc234b/raw/18df7964f384b84aabb2ac0649fb8dbd1bd254a9/trailing-space-in-comma-3547.php > a.php
cat a.php
php-cs-fixer --version
php-cs-fixer fix .
cat a.php

logs

5:24:50 ➜  /tmp  rm -rf /tmp/cs-fix
5:24:57 ➜  /tmp  mkdir /tmp/cs-fix
5:25:06 ➜  /tmp  cd /tmp/cs-fix
5:25:11 ➜  cs-fix  curl https://gist.githubusercontent.com/dbaltas/18befe7f6582b10168557a0fcdcc234b/raw/18df7964f384b84aabb2ac0649fb8dbd1bd254a9/trailing-space-in-comma-3547.php > a.php
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100   119  100   119    0     0    182      0 --:--:-- --:--:-- --:--:--   182
5:25:20 ➜  cs-fix  cat a.php
<?php

function foo() : void
{
    // there is a trailing space in this comment!
    $var1 = (
        $var2 ?? 0);
}
5:25:30 ➜  cs-fix  php-cs-fixer --version
PHP CS Fixer 2.12.1 Long Journey by Fabien Potencier and Dariusz Ruminski
5:25:44 ➜  cs-fix  php-cs-fixer fix .
Loaded config default.
   1) a.php

Fixed all files in 0.008 seconds, 10.000 MB memory used
5:25:53 ➜  cs-fix  cat a.php
<?php

function foo() : void
{
    // there is a trailing space in this comment!
    $var1 = (
        $var2 ?? 0

    );
}
5:25:57 ➜  cs-fix

Above is the terminal log of creating a file /tmp/cs-fix/a.php with "the before running" version.
After running php-cs-fixer fix, a new empty line has been added after the $var2 ?? 0.

I wouldn't expect the operating system to affect this. (I am running on MacOS 10.13.4, php 7.1.8)

@SpacePossum SpacePossum reopened this Jun 22, 2018
@SpacePossum
Copy link
Contributor

SpacePossum commented Jun 22, 2018

Thanks for getting back!
Seems this is indeed still an issue, my apologies for closing it early :(

--edit--
ignore below, see #3839 for details about the bug

After digging around this seems a priority issue between;
method_argument_space and no_trailing_whitespace_in_comment,

When no_trailing_whitespace_in_comment is run before method_argument_space it goes wrong (these both have priority 0)

However this does not happen when not using the rules default configuration;
something like this goes ok:
$ php php-cs-fixer fix tmp/test2.php -vvv --dry-run --diff --rules=method_argument_space,no_trailing_whitespace

but it does go wrong for something like this fails (note: no config file used);
$ php php-cs-fixer fix tmp/test2.php -vvv --dry-run --diff

so the priority issue is somewhere between the two rules and their configuration... bug-hunt continues!

keradus added a commit that referenced this issue Jul 6, 2018
…ePossum)

This PR was squashed before being merged into the 2.12 branch (closes #3839).

Discussion
----------

MethodArgumentSpaceFixer - add empty line incorrectly

closes #3457

Commits
-------

d7d8e61 MethodArgumentSpaceFixer - add empty line incorrectly
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

3 participants