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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

SingleSpaceAfterConstructFixer - Introduction #4435

Open
wants to merge 46 commits into
base: master
from

Conversation

@localheinz
Copy link
Contributor

localheinz commented Jun 3, 2019

This PR

  • introduces a SingleSpaceAfterConstructFixer

Fixes #3981.

馃拋鈥嶁檪 Not sure about this one, there are a quite a bunch of fixers which are dealing with spaces themselves, what do you think?

Handles

  • break
  • case
  • clone
  • continue
  • echo
  • goto
  • include
  • include_once
  • new
  • print
  • require
  • require_once
  • return
  • throw
  • yield
  • yield from

Does not handle

  • catch (handled by braces fixer)
  • do (handled by braces fixer)
  • else (handled by braces fixer)
  • elseif (handled by braces fixer)
  • if (handled by braces fixer)
  • switch (handled by braces fixer)
  • try (handled by braces fixer)
  • while (handled by braces fixer)
T_NEW,
T_PRINT,
T_RETURN,
T_THROW,

This comment has been minimized.

Copy link
@localheinz

localheinz Jun 3, 2019

Author Contributor

Could list a bunch of additional statements here, however, some of these statements are already handled by other fixers, for example:

diff --git a/src/Fixer/LanguageConstruct/SingleSpaceAfterConstructFixer.php b/src/Fixer/LanguageConstruct/SingleSpaceAfterConstructFixer.php
index d8864775..c71815e4 100644
--- a/src/Fixer/LanguageConstruct/SingleSpaceAfterConstructFixer.php
+++ b/src/Fixer/LanguageConstruct/SingleSpaceAfterConstructFixer.php
@@ -28,12 +28,27 @@ final class SingleSpaceAfterConstructFixer extends AbstractFixer
      */
     private static $tokenKinds = [
         T_BREAK,
+        T_CASE,
+        T_CATCH,
         T_CONTINUE,
+        T_DO,
         T_ECHO,
+        T_ELSE,
+        T_ELSEIF,
+        T_FOR,
+        T_FOREACH,
+        T_IF,
+        T_INCLUDE,
+        T_INCLUDE_ONCE,
         T_NEW,
         T_PRINT,
+        T_REQUIRE,
+        T_REQUIRE_ONCE,
         T_RETURN,
+        T_SWITCH,
         T_THROW,
+        T_TRY,
+        T_WHILE,
     ];

     /**

What do you think?

This comment has been minimized.

Copy link
@localheinz

localheinz Jun 3, 2019

Author Contributor

I think that for consistency, all of these should be added to the fixer. This should probably even belong to the @PSR2 ruleset, and then all of the other fixers would not need to worry about whitespace adjustment.

What do you think?

This comment has been minimized.

Copy link
@julienfalque

julienfalque Jun 3, 2019

Member

Indeed, PSR-2 requires to have a single space after most of those keywords:

Control structure keywords MUST have one space after them

Though I don't think this applies to e.g. echo or print. Maybe the list should be configurable so that the @PSR2 ruleset enables the fixer with the tokens related to this rule only?

This comment has been minimized.

Copy link
@localheinz

localheinz Jun 3, 2019

Author Contributor

Sounds good, @julienfalque!

@localheinz localheinz force-pushed the localheinz:feature/single-space-after-construct branch from 9550391 to 6a39e53 Jun 3, 2019
'return' => T_RETURN,
'throw' => T_THROW,
'yield' => T_YIELD,
'yield_from' => T_YIELD_FROM,

This comment has been minimized.

Copy link
@localheinz

localheinz Jun 3, 2019

Author Contributor

Let me know if you have any ideas for constructs (and/or statements) I could add here!

This comment has been minimized.

Copy link
@derrabus

derrabus Jul 4, 2019

Contributor

require, include and their *_once counterparts.

This comment has been minimized.

Copy link
@localheinz

localheinz Jul 4, 2019

Author Contributor

Thank you, @derrabus!

This comment has been minimized.

Copy link
@dmvdbrugge

dmvdbrugge Sep 16, 2019

Contributor

clone?

This comment has been minimized.

Copy link
@localheinz

localheinz Nov 7, 2019

Author Contributor

@derrabus

Implemented for

  • include
  • include_once
  • require
  • require_once

This comment has been minimized.

Copy link
@localheinz

localheinz Nov 7, 2019

Author Contributor

@dmvdbrugge

Implemented for

  • clone
@keradus keradus changed the title SingleSpaceAfterConstructFixer - Introduction Add SingleSpaceAfterConstructFixer Jun 3, 2019
@keradus keradus changed the base branch from 2.15 to master Jun 3, 2019
@keradus

This comment has been minimized.

Copy link
Member

keradus commented Jun 3, 2019

btw, CI is constantly failing on broken AutoReview

@localheinz localheinz force-pushed the localheinz:feature/single-space-after-construct branch 2 times, most recently from e9bb53d to 1fde7a4 Jun 3, 2019
@localheinz

This comment has been minimized.

Copy link
Contributor Author

localheinz commented Jun 3, 2019

Not sure how to deal with the failure related to the constant T_YIELD_FROM not defined on PHP 5.6, what do you think?

@localheinz

This comment has been minimized.

Copy link
Contributor Author

localheinz commented Jul 5, 2019

A bunch of tests are currently missing.

@IllyaMoskvin

This comment has been minimized.

Copy link

IllyaMoskvin commented Jul 21, 2019

Just want to say that this seems like a great feature. I'm new to using this tool, so I might have missed something, but the only other way I've found to fix things like if( to if ( is by using the braces fixer. I'm hesitant to do so due to how many responsibilities it has (cf. #472, #823). I think this is a nice addition for users who'd like to make those choices in a more granular manner.

@IllyaMoskvin

This comment has been minimized.

Copy link

IllyaMoskvin commented Jul 21, 2019

Found what looks like a bug. Running the fixer on this file:

<?php

if (1 === 1)
{
    echo 'foo';
}
else
{
    echo 'bar';
}

...will produce the following output:

<?php

if (1 === 1)
{
    echo 'foo';
}
else {
    echo 'bar';
}

Uhh please ignore the non-standard brace style, we don't usually do this. Just spotted it in the diff. It feels like removing the newline after the else is beyond the intended scope of this fixer.

@IllyaMoskvin

This comment has been minimized.

Copy link

IllyaMoskvin commented Jul 21, 2019

Another bug:

<?php

return;

...results in this:

<?php

return ;

This seems to impact break and continue the most. I can see adding spaces being useful for these constructs in some cases (e.g. continue(2)). Just thinking of scope. Can we make it so that there's no space added if the construct is immediately followed by a semicolon?

@localheinz

This comment has been minimized.

Copy link
Contributor Author

localheinz commented Jul 21, 2019

@IllyaMoskvin

Thank you for reporting!

This definitely needs work. I鈥檓 on vacation right now, will take a look when back behind my computer!

@localheinz localheinz force-pushed the localheinz:feature/single-space-after-construct branch from f5fb3ed to b702472 Aug 5, 2019
README.rst Outdated
- ``constructs`` (a subset of ``['break', 'continue', 'echo', 'goto', 'new',
'print', 'return', 'throw', 'yield', 'yield_from']``): list of constructs
which must be followed by a single space; defaults to ``['break',
'continue', 'goto', 'new', 'return', 'throw', 'yield']``

This comment has been minimized.

Copy link
@dmvdbrugge

dmvdbrugge Sep 16, 2019

Contributor

Why is yield default, but yield from not?

This comment has been minimized.

Copy link
@localheinz

localheinz Nov 7, 2019

Author Contributor

@dmvdbrugge

Currently failing because of support for PHP 5.6 - not sure yet how to work around it.

This comment has been minimized.

Copy link
@dmvdbrugge

dmvdbrugge Nov 7, 2019

Contributor

By only adding them if the version allows? There are more fixers with version-specific configuration.

localheinz added 11 commits Jun 3, 2019
localheinz added 2 commits Nov 7, 2019
@localheinz localheinz force-pushed the localheinz:feature/single-space-after-construct branch from 949d6e0 to 0d88a44 Nov 7, 2019
localheinz added 3 commits Nov 7, 2019
@localheinz localheinz changed the title SingleSpaceAfterConstructFixer - Introduction [WIP] SingleSpaceAfterConstructFixer - Introduction Nov 8, 2019
@localheinz localheinz force-pushed the localheinz:feature/single-space-after-construct branch from bb318c3 to 6b38259 Nov 8, 2019
@localheinz

This comment has been minimized.

Copy link
Contributor Author

localheinz commented Nov 8, 2019

@derrabus @dmvdbrugge @julienfalque @IllyaMoskvin @keradus

Let's see if the tests pass (yield from requires PHP 7).

Can you think of something else, perhaps, which of these constructs should be defaults?

@dmvdbrugge

This comment has been minimized.

Copy link
Contributor

dmvdbrugge commented Nov 8, 2019

Opinion: I would say all of them should be default (even yield from if the running version allows it).

@dmvdbrugge

This comment has been minimized.

Copy link
Contributor

dmvdbrugge commented Nov 8, 2019

Also the remaining points in the initial comment, you ask

(should it?)

In my opinion: yes (surprise surprise 馃槣)

parent::configure($configuration);
if (70000 <= \PHP_VERSION_ID && \defined('T_YIELD_FROM')) {
self::$tokenMap = array_merge(self::$tokenMap, [

This comment has been minimized.

Copy link
@dmvdbrugge

dmvdbrugge Nov 8, 2019

Contributor

These are both "hardcoded" arrays. Doesn't it make more sense to directly add the values, instead of array_merge?

localheinz added 3 commits Nov 8, 2019
@localheinz localheinz force-pushed the localheinz:feature/single-space-after-construct branch from 9a532f9 to 414d7f8 Nov 8, 2019
@localheinz localheinz force-pushed the localheinz:feature/single-space-after-construct branch from 314ee16 to dd77d28 Nov 8, 2019
@localheinz localheinz force-pushed the localheinz:feature/single-space-after-construct branch from dd77d28 to 69ae9a5 Nov 8, 2019
@localheinz localheinz changed the title [WIP] SingleSpaceAfterConstructFixer - Introduction SingleSpaceAfterConstructFixer - Introduction Nov 8, 2019
@localheinz

This comment has been minimized.

Copy link
Contributor Author

localheinz commented Nov 8, 2019

Does anyone have an idea how to work around the tests failing on PHP 5.6?

See https://travis-ci.org/FriendsOfPHP/PHP-CS-Fixer/jobs/609203996#L791-L800:

1) PhpCsFixer\Tests\Console\Command\ReadmeCommandTest::testIfReadmeFileIsCorrect
README.rst file is not up to date! Do not modify it manually! Regenerate readme with command: `php php-cs-fixer readme > README.rst`.
Failed asserting that two strings are identical.
--- Expected
+++ Actual
@@ @@
     'goto', 'include', 'include_once', 'new', 'print', 'require',
-    'require_once', 'return', 'throw', 'yield']``): list of constructs which
-    must be followed by a single space; defaults to ``['break', 'case',
-    'clone', 'continue', 'echo', 'goto', 'include', 'include_once', 'new',
-    'print', 'require', 'require_once', 'return', 'throw', 'yield']``
+    'require_once', 'return', 'throw', 'yield', 'yield_from']``): list of
+    constructs which must be followed by a single space; defaults to
+    ``['break', 'case', 'clone', 'continue', 'echo', 'goto', 'include',
+    'include_once', 'new', 'print', 'require', 'require_once', 'return',
+    'throw', 'yield', 'yield_from']``
localheinz added 3 commits Nov 8, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can鈥檛 perform that action at this time.