feature: binary_operator_spaces - Revert change about => alignment and use option instead #6724
Conversation
| @@ -2312,9 +3009,25 @@ function foo () { | |||
| } | |||
| ', | |||
| ], | |||
| [ | |||
| '<?php | |||
There was a problem hiding this comment.
Test added for #6716 (comment)
| @@ -656,6 +733,10 @@ private function injectAlignmentPlaceholdersForArrow(Tokens $tokens, int $startA | |||
| // no need to analyze for `isBinaryOperator` (always true), nor if part of declare statement (not valid PHP) | |||
| // there is also no need to analyse the second arrow of a line | |||
| if ($token->isGivenKind(T_DOUBLE_ARROW) && $newLineFoundSinceLastPlaceholder) { | |||
| if ($yieldFoundSinceLastPlaceholder) { | |||
There was a problem hiding this comment.
To handle #6716 (comment)
When a yield was previously use, it's another scope/level.
| @@ -774,7 +859,11 @@ private function replacePlaceholders(Tokens $tokens, string $alignStrategy, stri | |||
| foreach ($lines as $index => $line) { | |||
| if (substr_count($line, $placeholder) > 0) { | |||
| $groups[$groupIndex][] = $index; | |||
| } elseif ('=>' !== $tokenContent) { | |||
| } elseif ( | |||
There was a problem hiding this comment.
The specific behavior won't be hardcoded for => but instead will be an option BY_SCOPE for alignment.
| @@ -1643,6 +1643,703 @@ public function testFixAlignDoubleArrow(string $expected, ?string $input = null) | |||
| } | |||
|
|
|||
| public static function provideAlignDoubleArrowCases(): array | |||
| { | |||
| return [ | |||
There was a problem hiding this comment.
The git diff is not great but I've duplicated provideAlignDoubleArrowCases into provideAlignScopedDoubleArrowCases with small diff I'll list.
| "foo" => "bar", | ||
| "foofoo" => 42, | ||
| ]), | ||
| "baz" => "OK", |
There was a problem hiding this comment.
In provideAlignScopedDoubleArrowCases, it's aligned with others arrows
| 1 => 2, | ||
| 22 => 3, | ||
| ], | ||
| 100 => [ |
There was a problem hiding this comment.
In provideAlignScopedDoubleArrowCases, it's aligned with others arrows
| 20 => 22, | ||
| 30 => 33, | ||
| 40 | ||
| => |
There was a problem hiding this comment.
In provideAlignScopedDoubleArrowCases, it's aligned with others arrows
| 1 => 2, | ||
| 5 => [ |
There was a problem hiding this comment.
In provideAlignScopedDoubleArrowCases, it's aligned with others arrows
VincentLanglet
force-pushed
the
binaryOperator
branch
3 times, most recently
from
732ab12
to
ae40134
Compare
VincentLanglet
force-pushed
the
binaryOperator
branch
from
ae40134
to
55775d3
Compare
Pull Request Test Coverage Report for Build 3770962655
💛 - Coveralls |
|
Thank you! This is awesome.
I'm not good at English, but I'm fine with |
|
Hi, @julienfalque @keradus |
|
We are really waiting for this fix. |
|
We really need this fix. I thought the new organization was not good. |
|
TBH this PR is too big complex to find time to properly review it. As you were touching it last, I put my trust on you here. Big thanks for taking care of it, and sorry for delay from our side! |
keradus
changed the title
keradus
changed the title
Sure, I understand. I tried my best to cover changes with tests. Feel free to ping me if an issue occurs on this rule. |
HI @julienfalque
The previous bugfix in #6590 is subject to debate about yes or no it was a bug.
The issue #4395 and #5546 consider it's a bug.
The issue #6716 consider it's not.
I think the best solution is
align,align_single_space,align_single_space_minimalalign_by_scope,align_single_space_by_scope,align_single_space_minimal_by_scopeThis closes #6716
The only doubt I have is if
by_scopeis a good suffix or if there is one more meaningful...