Changed Yaml Dumper nested collection indentation #2242
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
fabpot
added a commit
that referenced
this pull request
Sep 23, 2011
Commits ------- 6d8c4a8 change nested collection indentation from 2 to 4 Discussion ---------- Changed Yaml Dumper nested collection indentation This PR changes the dumpers nested collection indentation from 2 to 4 which seems to be the standard.
sun
added a commit
to sun/symfony
that referenced
this pull request
Aug 5, 2012
…ion level of nested nodes. YAML does not specify an absolute indentation level, but a consistent indentation of nested nodes only: http://www.yaml.org/spec/1.2/spec.html#space/indentation/ Projects that are generally using 2 spaces for indentation should be able to retain consistency with their coding standards by supplying a custom value for the new $indent parameter added to Yaml::dump(), or the new Dumper::setIndentation() method. The new parameter is a backwards-compatible API addition and defaults to the previous default of 4 (which was changed from 2 via PR symfony#2242 only recently).
fabpot
added a commit
that referenced
this pull request
Aug 7, 2012
Commits ------- 3e1a1ab Force the value of Dumper::setIndentation($num) to be of type integer. 5be7237 Added Yaml\Dumper::setIndentation() method to allow a custom indentation level of nested nodes. Discussion ---------- [Yaml] Allow custom indentation level for nested nodes YAML does not specify an absolute indentation level, but a consistent indentation of nested nodes only: http://yaml.org/spec/current.html#indentation%20space/ Projects that are generally using 2 spaces for indentation should be able to retain consistency with their coding standards by supplying a custom value for the new $indent parameter for Yaml::dump() and Dumper::dump(). The new parameter is a backwards-compatible API addition and defaults to the previous default of 4 (which was changed from 2 via PR #2242 only recently). The old $indent parameter is renamed to $level, and remains to be used internally only. --------------------------------------------------------------------------- by travisbot at 2012-08-03T00:24:22Z This pull request [fails](http://travis-ci.org/symfony/symfony/builds/2024289) (merged 56331202 into b1618d2). --------------------------------------------------------------------------- by travisbot at 2012-08-03T00:29:02Z This pull request [fails](http://travis-ci.org/symfony/symfony/builds/2024315) (merged eeae28a4 into b1618d2). --------------------------------------------------------------------------- by travisbot at 2012-08-03T02:41:42Z This pull request [passes](http://travis-ci.org/symfony/symfony/builds/2024905) (merged 9a82c438 into b1618d2). --------------------------------------------------------------------------- by fabpot at 2012-08-03T07:53:50Z This is indeed a BC break as the method signature changed in a non-BC way. --------------------------------------------------------------------------- by fabpot at 2012-08-03T08:35:51Z I think I would prefer to have a static method to be able to change the number of spaces to use on a global basis. It makes more sense and would prevent the BC break. What do you think? --------------------------------------------------------------------------- by sun at 2012-08-03T13:23:23Z Thanks for your feedback! — Took some time to think through the static proposal. Here's what I think: 1. The call from DependencyInjection\Dumper\YamlDumper truly was a unexpected/nasty surprise for me. That is, because it passes a parameter to Yaml\Dumper that is documented for internal use only, and the surrounding YamlDumper code generally hard-codes lots of assumptions of where exactly the nodes/collections will be output in the dumped YAML structure. (I think that code can be simplified, cleaned up, and made faster at the same time (hence the todo), but indeed, that probably belongs into a separate issue.) 1. The essential problem with a static is closely bound to that though; a static property value for the indentation would "stick" and thus hi-jack the DependencyInjection\Dumper\YamlDumper, as it hard-codes and thus expects 4 spaces (contrary to any custom indentation). 1. Most code uses Yaml\Yaml as utility/helper service directly, so there'd be no clean way to prime the static with a custom value, aside from subclassing the entire thing - in which case this entire issue would sorta become moot, because at the point you're subclassing, you can as well go the extra mile and replace the entire Yaml\Dumper::dump() to implant the custom indentation level... 1. Another option would be to use a non-static Yaml\Dumper::$indent property, supplied through the constructor; i.e.: public function __construct($indent = 4) ...or alternatively, ::setIndentation(). Essentially requiring people to use and instantiate Yaml\Dumper directly, if they want to use a custom indentation. 1. Though in the end, I don't want to sound pedantic, but I *do* wonder a bit about the exact extent of the `@api` tags, as well as `@param`s that are explicitly documented as "internal use only"... :) That is, because only Yaml\Yaml is tagged with `@api`, but nothing in Yaml\Dumper. The same sorta applies to DependencyInjection\Dumper\YamlDumper::dump(), which is tagged with `@api`, but the :.addService() method being adjusted accordingly here is not. So essentially, when taking those tags (plus the param's description) seriously and in a nitpicky way, then there is no BC break, since no one should rely on their exact implementation... ;) (I perfectly realize that this is a long shot :)) So... depending on the final stance on the last point, I'd either move forward with the current proposal in this PR. Otherwise, I'd suggest the non-static property on Yaml\Dumper - in which case we'd likely try to swap out the static Symfony\Component\Yaml\Yaml helper with a Drupal\Component\Yaml\Yaml in order to always instantiate the dumper with the custom indentation. What do you think? --------------------------------------------------------------------------- by sun at 2012-08-04T22:57:21Z Alright. While I believe I made some good points in my last comment, I've taken the fully backwards-compatible path: - added the new $indent parameter only to Yaml::dump() - added a new Dumper::$intendation property and Dumper::setIndentation() method, to control the indentation level within the scope of a single Dumper instance only. Do you think this is acceptible? :) --------------------------------------------------------------------------- by travisbot at 2012-08-05T06:16:22Z This pull request [fails](http://travis-ci.org/symfony/symfony/builds/2039120) (merged 5be7237 into c99f9d2). --------------------------------------------------------------------------- by travisbot at 2012-08-07T07:51:04Z This pull request [fails](http://travis-ci.org/symfony/symfony/builds/2055164) (merged 3e1a1ab into c99f9d2). --------------------------------------------------------------------------- by sun at 2012-08-07T07:53:18Z Only one environment failed, and the [test failure](http://travis-ci.org/#!/symfony/symfony/jobs/2055165/L203) seems unrelated to this PR.
fabpot
pushed a commit
that referenced
this pull request
Jan 17, 2013
…ion level of nested nodes. YAML does not specify an absolute indentation level, but a consistent indentation of nested nodes only: http://www.yaml.org/spec/1.2/spec.html#space/indentation/ Projects that are generally using 2 spaces for indentation should be able to retain consistency with their coding standards by supplying a custom value for the new $indent parameter added to Yaml::dump(), or the new Dumper::setIndentation() method. The new parameter is a backwards-compatible API addition and defaults to the previous default of 4 (which was changed from 2 via PR #2242 only recently). Conflicts: src/Symfony/Component/Yaml/Dumper.php src/Symfony/Component/Yaml/Yaml.php
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.
This PR changes the dumpers nested collection indentation from 2 to 4 which seems to be the standard.