Added SingleClassElementPerStatementFixer #1889
Conversation
| * | ||
| * @author Javier Spagnoletti <phansys@gmail.com> | ||
| */ | ||
| final class SingleClassPropertyDeclarationFixer extends AbstractFixer |
There was a problem hiding this comment.
We have the SingleImportPerStatementFixer. So I think this fixer should be named SingleClassPropertyPerStatementFixer.
There was a problem hiding this comment.
Updated fixer name.
|
Thanks for working on this! I would like to see the same for class constants. |
|
What do you mean @gharlan? AFAIK this syntax isn't supported for class constants. |
5516133 to
717d3eb
Compare
|
| * | ||
| * @author Javier Spagnoletti <phansys@gmail.com> | ||
| */ | ||
| final class SingleClassPropertyDeclaredPerStatementFixer extends AbstractFixer |
There was a problem hiding this comment.
hmm, my proposal was SingleClassPropertyPerStatementFixer, without "Declared"
|
@gharlan, yesterday when I've read your message I've tested that syntax in 3v4l.org too and I'm sure that failed (I don't know what can I've tested now). |
SingleClassPropertyDeclarationFixersingle_property_per_statement
|
@phansys same fixer, different configuration |
|
@keradus, it's possible to attach just a fixer configuration to a ruleset (PSR-2 in this case)? |
|
yes |
|
|
any news here ? |
|
I'm very busy these days @keradus, my apologies. I'll try to find some time on weekend in order to continue this task. BTW, if somebody else wants to put some effort on it, I'll be very glad. |
|
Thanks for the time you're dedicating to this @SpacePossum. I totally agree if you want to provide a PR in replacement of mine. Please let me know. |
|
@phansys if you able to finish this PR it would be great ! @SpacePossum thanks for your willingness for help, but please focus on currently opened PR of yours, like PHP7 bugfixes for 1.11 line ! I would say it's more important. |
|
btw, please regenerate the readme ;) |
|
@phansys I have no time to take this one over, but I'm happy to help out or doing reviewing.
good luck! |
|
btw. ping @gharlan I've added your sample case, can you confirm this is what you meant? |
yes, thanks! |
| use PhpCsFixer\Test\AbstractFixerTestCase; | ||
|
|
||
| /** | ||
| * @author Javier Spagnoletti <phansys@gmail.com> |
|
Test are green now @keradus. |
|
@phansys @SpacePossum |
|
I'll try to check this fixer today against some projects as @SpacePossum suggested. |
|
I've renamed the fixer, added some other tests and opened a PR against Symfony in order to get some feedback. |
| <<<'EOT' | ||
| <?php | ||
|
|
||
| class Foo | ||
| { | ||
|
|
||
| public $bar = null; | ||
|
|
There was a problem hiding this comment.
Which is your concern about this removed LF @GrahamCampbell?
There was a problem hiding this comment.
Yes, I'm asking why this was removed.
There was a problem hiding this comment.
Because this fixer now uses just the first previous line break as separator, based on feedback from Symfony.
single_property_per_statementsingle_class_element_per_statement
This PR was merged into the 2.7 branch. Discussion ---------- [CS] Respect PSR2 4.2 | Q | A | ------------- | --- | Branch | 2.7 | Bug fix? | no | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | | License | MIT | Doc PR | http://www.php-fig.org/psr/psr-2/#4-2-properties I'm writing a [new fixer](PHP-CS-Fixer/PHP-CS-Fixer#1889) (`single_class_element_per_statement`), which intends to avoid multiple class elements in one statement. This PR is a POC about the result of this fixer, so I'd like to know what do you think about, since it will be also added to the `@Symfony` ruleset. Please note that class constants are considered too, but here isn't any offending code about that. Example output: ``` $ ./php-cs-fixer.phar fix . --rules=single_class_element_per_statement -vvv 1) src/Symfony/Component/Console/Tests/Input/InputDefinitionTest.php (single_class_element_per_statement) 2) src/Symfony/Component/DependencyInjection/Tests/ContainerTest.php (single_class_element_per_statement) 3) src/Symfony/Component/DependencyInjection/Tests/Fixtures/includes/foo.php (single_class_element_per_statement) 4) src/Symfony/Bridge/ProxyManager/Tests/LazyProxy/Fixtures/includes/foo.php (single_class_element_per_statement) ``` Commits ------- d250b1d [CS] Respect PSR2 4.2
| $divisionContent = null; | ||
| if ($tokens[$startIndex - 1]->isWhitespace()) { | ||
| $divisionContent = $tokens[$startIndex - 1]->getContent(); | ||
| if (preg_match('#(\n|\n\r)#', $divisionContent, $matches)) { |
There was a problem hiding this comment.
The sequence is typically \r\n IIRC, but it shouldn't matter, i.e. both cases should match.
So I think the match should be something like ( 0-N times \r 1-N times \n 0-N times \r ).
| @@ -144,7 +142,9 @@ private function expandElement(Tokens $tokens, $startIndex, $endIndex) | |||
| if ($divisionContent) { | |||
| $tokens->insertAt($i + 1, new Token(array(T_WHITESPACE, $divisionContent))); | |||
| } | |||
| $tokens->insertAt($i + 2, $modifiers); | |||
| // collect modifiers | |||
| $sequence = $this->getModifiersSequences($tokens, $startIndex, $endIndex); | |||
There was a problem hiding this comment.
I'm not sure that calling getModifiersSequences method in loop is good, as it's already iterate through tokens in loop as well
|
Could you squash consecutive commits of same author please? |
| Q | A | ------------- | --- | Branch | master | Bug fix? | no | New feature? | yes | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | PHP-CS-Fixer#1839 | License | MIT | Doc PR |
|
Of course @keradus, done. |
|
👍 |
single_class_element_per_statementsingle_class_element_per_statement
|
👍 |
single_class_element_per_statement…cePossum) This PR was merged into the 2.0-dev branch. Discussion ---------- Added SingleClassElementPerStatementFixer | Q | A | ------------- | --- | Branch | master | Bug fix? | no | New feature? | yes | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #1839 | License | MIT | Doc PR | According to PSR-2: >There MUST NOT be more than one property declared per statement. Commits ------- c18509c Fix a329745 [PSR-2] Add `SingleClassElementPerStatementFixer`
|
Thank you @phansys and @SpacePossum ! Great work !!! |
According to PSR-2: