Fix Tokens::insertSlices not moving around all affected tokens
#6058
Conversation
|
looks promising,yet like to have @keradus to take look if he has time |
|
reviewed, left some suggestions here paulbalandan#1 |
SpacePossum
force-pushed
the
tokens-insert-slices-multiple
branch
from
085af4e
to
fbac555
Compare
|
Thank you @paulbalandan. |
| @@ -859,20 +860,28 @@ public function insertSlices(array $slices): void | |||
|
|
|||
| krsort($slices); | |||
|
|
|||
| if (array_key_first($slices) > $oldSize) { | |||
| throw new \OutOfBoundsException('Cannot insert outside of collection.'); | |||
There was a problem hiding this comment.
I'm not convinced it's good validation, as it's checking only index for first slice. And it looks like there are not tests checking it? :(
ref #6172
There was a problem hiding this comment.
we sort the keys above it, so we know all other keys are of a lower value so we only need to test the first,
all others are tested to be greater or equal to zero
| if ($this->changed) { | ||
| return true; | ||
| } | ||
|
|
||
| return false; | ||
| return $this->changed; |
There was a problem hiding this comment.
i remember the idea here was to have different lines for different paths, so we can have explicit code coverage not just by one-liner covered, but to ensure both paths are covered for this crucial core-level functionality
@SpacePossum , should we move back to multiline approach, perhaps?
There was a problem hiding this comment.
I'm not sure I follow, what is the benefit of higher LOC's here?
There was a problem hiding this comment.
i remember initial reasoning. we wanted to ensure, initially, that we don't test only cases like "this->changed=true"
There was a problem hiding this comment.
oh like in the coverage it won't show if both true and false and checked.... that is true. I value a more direct and compact notation even if that means a bit less verbose coverage report.
| <?php | ||
|
|
||
| $after = get_class($after); | ||
| $before = get_class($before); |
There was a problem hiding this comment.
this case is wrong, we must avoid having whitespace next to whitespace as 2 separated tokens!
There was a problem hiding this comment.
somehow, in line 1398, we check only text-representation of collection. this is wrong, as to whitespace token should not be place one to each other (new Token([T_WHITESPACE, ' ']), new Token([T_WHITESPACE, ' ']))
after strong check of assertTokens, this case is failing d865593
While working on #5953 in refactoring to call
insertSlicesonce, I stumbled into a problem where parse error would appear after fixing. After digging into it more, I found out that the culprit is in theinsertSlicesmethod itself. If there are multiple slices to be inserted in an index and this is repeated in other indexes, there exists a "hole" left in the moving around of tokens. This causes the unmoved tokens to be inadvertently replaced by other else possibly resulting to parse errors after.