Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Yaml] Allow dumping empty array as YAML sequence #21471

Merged
merged 3 commits into from Feb 19, 2017

Conversation

Projects
None yet
5 participants
@c960657
Copy link
Contributor

commented Jan 31, 2017

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.

$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);

This comment has been minimized.

Copy link
@xabbuh

xabbuh Jan 31, 2017

Member

I would not expect an empty sequence as the result with this input.

This comment has been minimized.

Copy link
@c960657

c960657 Jan 31, 2017

Author Contributor

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);

This comment has been minimized.

Copy link
@xabbuh

xabbuh Jan 31, 2017

Member

same here

Christian Schmidt

@nicolas-grekas nicolas-grekas added this to the 3.x milestone Jan 31, 2017

@fabpot

This comment has been minimized.

Copy link
Member

commented Feb 16, 2017

@xabbuh I let you merge this one?

@xabbuh

This comment has been minimized.

Copy link
Member

commented Feb 17, 2017

@fabpot Sure, can I understand your comment as a +1 vote?

@@ -29,6 +29,7 @@ class Yaml
const DUMP_OBJECT_AS_MAP = 64;
const DUMP_MULTI_LINE_LITERAL_BLOCK = 128;
const PARSE_CONSTANT = 256;
const DUMP_EMPTY_ARRAY_AS_SEQUENCE = 512;

This comment has been minimized.

Copy link
@nicolas-grekas

nicolas-grekas Feb 17, 2017

Member

alpha order?

This comment has been minimized.

Copy link
@c960657

c960657 Feb 17, 2017

Author Contributor

Do you want me to sort all constants alphabetically? I don't see this convention being used elsewhere in Symfony.

This comment has been minimized.

Copy link
@xabbuh

xabbuh Feb 17, 2017

Member

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.

This comment has been minimized.

Copy link
@nicolas-grekas

nicolas-grekas Feb 17, 2017

Member

can't we change the values of the consts?

This comment has been minimized.

Copy link
@fabpot

fabpot Feb 17, 2017

Member

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.

This comment has been minimized.

Copy link
@xabbuh

xabbuh Feb 20, 2017

Member

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).

@@ -1,6 +1,15 @@
CHANGELOG
=========

3.2.3

This comment has been minimized.

Copy link
@nicolas-grekas

nicolas-grekas Feb 17, 2017

Member

wrong version

This comment has been minimized.

Copy link
@c960657

c960657 Feb 17, 2017

Author Contributor

Fixed.

@@ -1,6 +1,15 @@
CHANGELOG
=========

3.2.5

This comment has been minimized.

Copy link
@xabbuh

xabbuh Feb 17, 2017

Member

That's still not right. New features are only added in minor versions. So this needs to be 3.3.

This comment has been minimized.

Copy link
@c960657

c960657 Feb 17, 2017

Author Contributor

Fixed again :-)

@c960657 c960657 force-pushed the c960657:yaml-empty-array branch from 4376c7e to 87ffaf2 Feb 17, 2017

@fabpot

This comment has been minimized.

Copy link
Member

commented Feb 19, 2017

Thank you @c960657.

@fabpot fabpot closed this Feb 19, 2017

@fabpot fabpot merged commit 87ffaf2 into symfony:master Feb 19, 2017

1 of 2 checks passed

continuous-integration/appveyor/pr AppVeyor was unable to build non-mergeable pull request
Details
fabbot.io Your code looks good.
Details

fabpot added a commit that referenced this pull request Feb 19, 2017

feature #21471 [Yaml] Allow dumping empty array as YAML sequence (c96…
…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 number
af7067c Dump empty object as mapping
a6d94c1 [Yaml] Allow dumping empty array as YAML sequence

@nicolas-grekas nicolas-grekas modified the milestones: 3.x, 3.3 Mar 24, 2017

@fabpot fabpot referenced this pull request May 1, 2017

Merged

Release v3.3.0-BETA1 #22603

@c960657 c960657 deleted the c960657:yaml-empty-array branch May 22, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.