[2.1][Component][Yaml] fix 4022 #4126
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| if ( | ||
| $this->getCurrentLineIndentation() == $currentIndentation | ||
| && | ||
| preg_match('/^- .*/', $this->currentLine) |
There was a problem hiding this comment.
matching .* makes no sense because it could simply be removed without any impact.
There was a problem hiding this comment.
Yes. We only need to check, that string begins with a dash followed by a space. This is now done with strpos().
|
Why is it marked as @fabpot ping |
|
The title should be [2.1] - now it is correct. I marked it 2.0 and PR was for 2.0 originally. Fabien suggested that it should go to master branch: #4121 (comment) |
| $unindentedEmbedBlock = true; | ||
| } | ||
|
|
||
| if (!$this->isCurrentLineEmpty() && 0 == $newIndent && !$unindentedEmbedBlock) { |
|
That does not work when you have something after the unindented collection: |
|
@fabpot Last commit contains test with your yaml: Everything seems fine. Can you give me a hint: what do you mean, when you say "That does not work"? |
|
Sorry, the failing test is the following: |
|
@fabpot Last commit passed your test. |
|
Can you squash your commits? Thanks. |
| } | ||
|
|
||
| $ret = false; | ||
| if ( |
There was a problem hiding this comment.
extra line breaks after ( and before )
There was a problem hiding this comment.
When the condition is as long as:
if ($this->getCurrentLineIndentation() == $currentIndentation && $this->isStringUnIndentedCollectionItem($this->currentLine)) {
multiline notation is much easier to read. Should I change it into one long line?
|
Done. |
fabpot
added a commit
that referenced
this pull request
May 8, 2012
Commits ------- 80a2a92 [2.1][Component][Yaml] fix 4022 Discussion ---------- [2.1][Component][Yaml] fix 4022 Bug fix: yes Feature addition: no Backwards compatibility break: no Symfony2 tests pass: [](http://travis-ci.org/gajdaw/symfony) Fixes the following tickets: #4121, #4022, #4135 Todo: --------------------------------------------------------------------------- by stof at 2012-04-27T13:03:15Z Why is it marked as ``[2.2]`` if it is a bugfix ? @fabpot ping --------------------------------------------------------------------------- by gajdaw at 2012-04-27T14:42:21Z The title should be [2.1] - now it is correct. I marked it 2.0 and PR was for 2.0 originally. Fabien suggested that it should go to master branch: #4121 (comment) --------------------------------------------------------------------------- by fabpot at 2012-05-07T09:17:31Z That does not work when you have something after the unindented collection: collection: key: - a - b - c foo: bar --------------------------------------------------------------------------- by gajdaw at 2012-05-07T11:11:30Z @fabpot Last commit contains test with your yaml: collection: key: - a - b - c foo: bar Everything seems fine. Can you give me a hint: what do you mean, when you say "That does not work"? --------------------------------------------------------------------------- by fabpot at 2012-05-07T12:36:19Z Sorry, the failing test is the following: test: Key/value after unindented collection brief: > Key/value after unindented collection yaml: | collection: key: - a - b - c foo: bar php: | array('collection' => array('key' => array('a', 'b', 'c'), 'foo' => 'bar')) --------------------------------------------------------------------------- by gajdaw at 2012-05-07T15:48:26Z @fabpot Last commit passed your test. --------------------------------------------------------------------------- by fabpot at 2012-05-07T17:28:21Z Can you squash your commits? Thanks. --------------------------------------------------------------------------- by travisbot at 2012-05-08T05:32:58Z This pull request [fails](http://travis-ci.org/symfony/symfony/builds/1273487) (merged 20891c58 into 919604a). --------------------------------------------------------------------------- by gajdaw at 2012-05-08T05:36:51Z Done. --------------------------------------------------------------------------- by travisbot at 2012-05-08T07:23:47Z This pull request [passes](http://travis-ci.org/symfony/symfony/builds/1274162) (merged 80a2a92 into 898ff4e).
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Bug fix: yes
Feature addition: no
Backwards compatibility break: no
Symfony2 tests pass:
Fixes the following tickets: #4121, #4022, #4135
Todo: