Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[Yaml] Fix wrong line number when comments are inserted in the middle of a block.#17733
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
xabbuh commentedFeb 9, 2016
@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? |
paradajozsef commentedFeb 11, 2016
@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. |
paradajozsef commentedFeb 13, 2016
@xabbuh I changed the fix. The above test case passes, failing VarDumper tests unrelated imo. |
paradajozsef commentedFeb 13, 2016
Status: Needs Review |
| * | ||
| * @return bool | ||
| */ | ||
| privatefunctionisPreviousNonCommentLineIsCollectionItem() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
method name is weird. It containsis twice
fabpot commentedApr 28, 2016
ping@xabbuh |
paradajozsef commentedApr 30, 2016
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 |
xabbuh commentedMay 6, 2016
@paradajozsef Can you please rebase and push again? Looks like Travis otherwise will not run builds for your changes otherwise. |
paradajozsef commentedMay 6, 2016
Sure, I can do it tonight. |
stof commentedMay 6, 2016
@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 tobf4501aCompareparadajozsef commentedMay 6, 2016
Rebase is done, tests are green. |
| { | ||
| $isCollectionItem =false; | ||
| $moves =0; | ||
| while($this->moveToPreviousLine()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
please add a space before the opening parenthesis (right after thewhile keyword)
xabbuh commentedMay 10, 2016
👍 Status: Reviewed ping @symfony/deciders |
paradajozsef commentedMay 11, 2016
@xabbuh Thx for the review, I fixed them. |
fabpot commentedJun 8, 2016
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.)