[Yaml] Fix wrong line number when comments are inserted in the middle of a block. #17733
Conversation
|
@paradajozsef I think the fix is a bit more complex. For example, I do not think that this will solve the issue for the following example too: Can you add it as a test case too? |
|
@xabbuh Sorry for late answer. I added the test case and as always you are right again. 😃 In this case the line number is 9, but it should be 8. 😕 Still needs work. |
|
@xabbuh I changed the fix. The above test case passes, failing VarDumper tests unrelated imo. |
|
Status: Needs Review |
| * | ||
| * @return bool | ||
| */ | ||
| private function isPreviousNonCommentLineIsCollectionItem() |
There was a problem hiding this comment.
method name is weird. It contains is twice
|
ping @xabbuh |
|
I apologize to everyone for late answer. (I've been lack of internet at home lately because of moving to another place.) @stof Thanks for the review. I changed the comment and method name. What about |
|
@paradajozsef Can you please rebase and push again? Looks like Travis otherwise will not run builds for your changes otherwise. |
|
Sure, I can do it tonight. |
|
@xabbuh the reason why Travis does not run tests is because the branch conflicts. As Travis is actually running PR tests on a merge commit rather than on the feature branch, it cannot run them in case of conflicts |
adb111d to
bf4501a
Compare
|
Rebase is done, tests are green. |
| { | ||
| $isCollectionItem = false; | ||
| $moves = 0; | ||
| while($this->moveToPreviousLine()) { |
There was a problem hiding this comment.
please add a space before the opening parenthesis (right after the while keyword)
|
👍 Status: Reviewed ping @symfony/deciders |
|
@xabbuh Thx for the review, I fixed them. |
|
Thank you @paradajozsef. |
… the middle of a block. (paradajozsef) This PR was submitted for the master branch but it was merged into the 2.7 branch instead (closes #17733). Discussion ---------- [Yaml] Fix wrong line number when comments are inserted in the middle of a block. | Q | A | ------------- | --- | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #15437 | License | MIT | Doc PR | - @xabbuh what is your opinion a solution like this? (counting the skipped comment lines and when exception occurs add them to the current line count.) Commits ------- d83e346 [Yaml] Fix wrong line number when comments are inserted in the middle of a block.
This PR was merged into the 2.7 branch. Discussion ---------- [Yaml] properly count skipped comment lines | Q | A | ------------- | --- | Branch? | 2.7 | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #15437, #17733, #19023 | License | MIT | Doc PR | Commits ------- da7fc36 [Yaml] properly count skipped comment lines
@xabbuh what is your opinion a solution like this? (counting the skipped comment lines and when exception occurs add them to the current line count.)