8000 merged branch sun/yaml-indent (PR #5157) · symfony/symfony@b91a4a8 · GitHub
[go: up one dir, main page]
Skip to content

Commit b91a4a8

Browse files
fabpotfabpot
committed
merged branch sun/yaml-indent (PR #5157)
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.
2 parents 13454ef + 3e1a1ab commit b91a4a8

File tree

2 files changed

+21
-2
lines changed

2 files changed

+21
-2
lines changed

‎src/Symfony/Component/Yaml/Dumper.php

Lines changed: 18 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,23 @@
1818
*/
1919
class Dumper
2020
{
21+
/**
22+
* The amount of spaces to use for indentation of nested nodes.
23+
*
24+
* @var integer
25+
*/
26+
protected $indentation = 4;
27+
28+
/**
29+
* Sets the indentation.
30+
*
31+
* @param integer $num The amount of spaces to use for intendation of nested nodes.
32+
*/
33+
public function setIndentation($num)
34+
{
35+
$this->indentation = (int) $num;
36+
}
37+
2138
/**
2239
* Dumps a PHP value to YAML.
2340
*
@@ -44,7 +61,7 @@ public function dump($input, $inline = 0, $indent = 0)
4461 E195
$prefix,
4562
$isAHash ? Inline::dump($key).':' : '-',
4663
$willBeInlined ? ' ' : "\n",
47-
$this->dump($value, $inline - 1, $willBeInlined ? 0 : $indent + 4)
64+
$this->dump($value, $inline - 1, $willBeInlined ? 0 : $indent + $this->indentation)
4865
).($willBeInlined ? "\n" : '');
4966
}
5067
}

‎src/Symfony/Component/Yaml/Yaml.php

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -97,14 +97,16 @@ public static function parse($input)
9797
*
9898
* @param array $array PHP array
9999
* @param integer $inline The level where you switch to inline YAML
100+
* @param integer $indent The amount of spaces to use for indentation of nested nodes.
100101
*
101102
* @return string A YAML string representing the original PHP array
102103
*
103104
* @api
104105
*/
105-
public static function dump($array, $inline = 2)
106+
public static function dump($array, $inline = 2, $indent = 4)
106107
{
107108
$yaml = new Dumper();
109+
$yaml->setIndentation($indent);
108110

109111
return $yaml->dump($array, $inline);
110112
}

0 commit comments

Comments
 (0)
0