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

Subarray of sequential array must be inline in Yaml #31691

Closed
wants to merge 2 commits into from
Closed

Subarray of sequential array must be inline in Yaml #31691

wants to merge 2 commits into from

Conversation

JJarrie
Copy link
Contributor

@JJarrie JJarrie commented May 29, 2019

Q A
Branch? 3.4
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets n/a
License MIT
Doc PR n/a

Hello,

As I investigated this issue, I noticed this rule which seems to me implicit to avoid generation like

foo:
    -
        bar: baz
        qux: quux

Instead of

foo:
    - { bar: baz, qux: quux }

Here is the most surgicals changes to obtain the expected result without breaking the existing output.

@xabbuh
Copy link
Member

xabbuh commented May 29, 2019

Can you explain why you think that the mapping should be dumped inline?

@JJarrie
Copy link
Contributor Author

JJarrie commented May 29, 2019

I think that the mapping should be dumped inline when it is into array for somes reasons :

1/ Readibility : the generation's example I posted is not really an exemple of good organization of code, especially for a configuration. At least the carriage is too much.

2/ Documentation: I haven't look for a while into documentation an example who give the multiline syntax.

3/ Seem Symfony's standard : In the whole project, except few files into tests, the inline syntax is used.

4/ Usage: With this update, this is quick and easy to fix the linked issue and enhance the Authenticator Maker, I think few others Maker will generate configuration's files with this syntax.

This is my opinion, sorry if I'm on the wrong way.

@nicolas-grekas nicolas-grekas added this to the 3.4 milestone May 29, 2019
@fabpot
Copy link
Member

fabpot commented Jul 8, 2019

Not sure I agree with the changes, but as this is not a bug fix, it should target 4.4. /cc @xabbuh

Copy link
Member

@nicolas-grekas nicolas-grekas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

makes sense to me

@xabbuh
Copy link
Member

xabbuh commented Jul 18, 2019

I am mixed here. I could imagine other users complaining about this change if they like the previous behaviour especially since you could get the output desired by this PR by changing the inline argument, right?

@stof
Copy link
Member

stof commented Jul 18, 2019

If the values in the subarray are more complex (nested arrays), the readability argument would precisely reject your change. So I'm also mixed here.

@fabpot
Copy link
Member

fabpot commented Jul 18, 2019

Let's close as 3 core team members are not convinced that the changes are worth it.

@fabpot fabpot closed this Jul 18, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants