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] Allow dumping empty array as YAML sequence#21471
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
| $dump =$this->dumper->dump(array(),9,0, Yaml::DUMP_EMPTY_ARRAY_AS_SEQUENCE); | ||
| $this->assertEquals('[]',$dump); | ||
| $dump =$this->dumper->dump(new \ArrayObject(),0,0, Yaml::DUMP_EMPTY_ARRAY_AS_SEQUENCE | Yaml::DUMP_OBJECT_AS_MAP); |
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.
I would not expect an empty sequence as the result with this input.
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.
Agreed. Fixed.
| $dump =$this->dumper->dump(new \ArrayObject(),0,0, Yaml::DUMP_EMPTY_ARRAY_AS_SEQUENCE | Yaml::DUMP_OBJECT_AS_MAP); | ||
| $this->assertEquals('[]',$dump); | ||
| $dump =$this->dumper->dump(new \stdClass(),0,0, Yaml::DUMP_EMPTY_ARRAY_AS_SEQUENCE | Yaml::DUMP_OBJECT_AS_MAP); |
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.
same here
fabpot commentedFeb 16, 2017
@xabbuh I let you merge this one? |
xabbuh commentedFeb 17, 2017
@fabpot Sure, can I understand your comment as a +1 vote? |
| constDUMP_OBJECT_AS_MAP =64; | ||
| constDUMP_MULTI_LINE_LITERAL_BLOCK =128; | ||
| constPARSE_CONSTANT =256; | ||
| constDUMP_EMPTY_ARRAY_AS_SEQUENCE =512; |
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.
alpha order?
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.
Do you want me to sort all constants alphabetically? I don't see this convention being used elsewhere in Symfony.
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.
I would keep it this way. Otherwise, it's becoming hard to add new flags as you need to be extra careful to not reuse one of the existing flag values.
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.
can't we change the values of the consts?
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.
I like the way it is now, the more recent addition are last with a higher number. I don't see why we should order them.
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.
Sorry for being late here, but I had no time during the weekend to do the final review. This value must be adjusted as 512 is already used by the new tags support feature (see#21678).
| CHANGELOG | ||
| ========= | ||
| 3.2.3 |
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.
wrong version
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.
Fixed.
| CHANGELOG | ||
| ========= | ||
| 3.2.5 |
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.
That's still not right. New features are only added in minor versions. So this needs to be3.3.
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.
Fixed again :-)
fabpot commentedFeb 19, 2017
Thank you@c960657. |
…0657)This PR was squashed before being merged into the 3.3-dev branch (closes#21471).Discussion----------[Yaml] Allow dumping empty array as YAML sequence| Q | A| ------------- | ---| Branch? | master| Bug fix? | no| New feature? | yes| BC breaks? | no| Deprecations? | no| Tests pass? | yes| Fixed tickets |#9870,#15937,#16266| License | MIT| Doc PR |PHP arrays are dumped as either YAML sequences or mappings, depending on whether the array has continuos integer keys or not.An empty array is always dumped as a YAML mapping. Sometimes you want it dumped as a YAML sequence instead.Commits-------87ffaf2 Bump version numberaf7067c Dump empty object as mappinga6d94c1 [Yaml] Allow dumping empty array as YAML sequence
Uh oh!
There was an error while loading.Please reload this page.
PHP arrays are dumped as either YAML sequences or mappings, depending on whether the array has continuos integer keys or not.
An empty array is always dumped as a YAML mapping. Sometimes you want it dumped as a YAML sequence instead.