[Translator] Dump translation constants as tree instead of simple list #14434

gepo · 2015-04-21T21:43:09Z

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

Dump translations constants as tree based on '.' character as a delimeter in path.

YamlFileDumper converts constants like these:

'foo.bar1' => 'test1',
'foo.bar2' => 'test2'

to next .yml file:

foo.bar1: 'test1'
foo.bar2: 'test2'

This PR convert the same input to this:

foo:
    bar1: 'test1'
    bar2: 'test2'

Which is more useful if you use translation import/export tools while storing translations in YAML files.

stof · 2015-04-21T22:35:29Z

src/Symfony/Component/Translation/Dumper/YamlFileDumper.php

@@ -36,4 +36,68 @@ protected function getExtension()
    {
        return 'yml';
    }
+
+    protected function expandToTree(array $messages)


methods should be protected (there is no reason to make them extension points yet)

stof · 2015-04-21T22:38:01Z

I think this should be configurable through an option

stloyd · 2015-04-22T06:54:29Z

src/Symfony/Component/Translation/Dumper/YamlFileDumper.php

+
+        foreach ($parts as $i => $part) {
+            if (! isset($elem[$part])) {
+                $elem[$part] = [];


Should be:

$elem[$part] = array();

aitboudad · 2015-04-22T07:13:59Z

This look nice addition, what do you think to move this feature into `yaml dumper' ?

jakzal · 2015-04-22T08:07:39Z

@aitboudad I wouldn't add this feature to the yaml component as it's not part of the yaml standard.

gepo · 2015-04-22T09:04:32Z

I think this should be configurable through an option

ok, sure, did it

stof · 2015-04-22T09:11:50Z

src/Symfony/Component/Translation/Dumper/YamlFileDumper.php

+     * 
+     * @param bool $asTree
+     */
+    public function setDumpAsTree($asTree)


This is not how it should work. It should rely on options passed to the dump method (see the interface)

FileDumper doesn't pass this options to format, so their several ways to do it:

Override dump() function, save options internally and reset them after format() call

Change FileDumper - add optional third parameter to format function and pass it in dump() function.

Copy-paste&edit dump function from FileDumper to YamlFileDumper (bad, very bad).

or leave it as setter function because if we need it dumping as tree we probably need it in whole application, not just in one case.

I think, way 2. is most suitable, but I'm not sure that it's not BC-break. Is this way ok?

I'd prefer 2. it's not a BC break but it must be documented in the UPGRADE file.

protected function format(MessageCatalogue $messages, $domain, $options = array());

with a typehint on the options argument

I think the BC break is fine by looking at implementations of this outside Symfony: http://packanalyst.com/class?q=Symfony\Component\Translation\Dumper\FileDumper

Drupal and Prezent are extending the YamlFileDumper to implement this feature, meaning their upgrade path will be easy: switch to the core one.

And then, we could even submit PRs ourselves to Sulu and Kdyby to update their implementations

I certantly can't just add third option to format() in FileDumper without BC break. With introducing two new abstract classes I gain full BC, check it pls. Also, I'm not sure about new class names...

todo:

cleanup tests

write doc block

update UPGRADE file

The new abstract class makes the component more complex without any real benefit IMO.

And if you want to make the tree logic reusable, the right way would be to put it in its own class used as a collaborator of the dumper instead of using an abstract parent class. It would be much more reusable (and easier to test as you could test the tree logic on its own btw)

gepo · 2015-04-22T22:10:37Z

Also add this behavior to json and php dumpers.

8000

stloyd · 2015-04-23T06:44:58Z

src/Symfony/Component/Translation/Dumper/AbstractTreeFileDumper.php

+    {
+        $data = $messages->all($domain);
+
+        if (array_key_exists('as_tree', $options) && $options['as_tree']) {


if (isset($options['as_tree'])) { }

gepo · 2015-04-25T09:34:19Z

squash all commits into one to easy review.

move expandToTree to separate class and use it only in YamlFileDumper

aitboudad · 2015-04-25T11:28:30Z

CHANGELOG-2.7.md

@@ -9,6 +9,7 @@ To get the diff between two versions, go to https://github.com/symfony/symfony/c

 * 2.7.0-BETA1 (2015-04-10)

+ * feature #14434 [Translation] add options 'as_tree', 'inline' to YamlFileDumper (gepo)


you should update CHANGELOG of Translation and the target should be 2.8

aitboudad · 2015-04-26T10:26:55Z

src/Symfony/Component/Translation/Dumper/YamlFileDumper.php

@@ -36,4 +47,17 @@ protected function getExtension()
    {
        return 'yml';
    }
+
+    /**
+     * This function provided for backward compatibility.


This function is already documented in FileDumper, so use inheritdoc

/** * {@inheritdoc} */

aitboudad · 2015-04-27T11:47:06Z

👍, ping @symfony/deciders

xabbuh · 2015-04-28T18:52:43Z

src/Symfony/Component/Translation/Dumper/FileDumper.php

+     *
+     * @param MessageCatalogue $messages
+     * @param string           $domain
+     * @param array            $options


array should be string[]

xabbuh · 2015-05-07T18:27:06Z

src/Symfony/Component/Translation/Dumper/YamlFileDumper.php

@@ -13,6 +13,7 @@

 use Symfony\Component\Translation\MessageCatalogue;
 use Symfony\Component\Yaml\Yaml;
+use Symfony\Component\Translation\Util\ArrayConverter;


Should be above the YAML component use statement.

It was fixed

xabbuh · 2015-05-12T19:23:12Z

👍

jakzal · 2015-05-13T06:22:34Z

src/Symfony/Component/Translation/Dumper/YamlFileDumper.php

+            return Yaml::dump($data, $inline);
+        }
+
+        return Yaml::dump($data);


this could be simplified:

$inline = isset($options['inline']) && $options['inline'] > 0; return Yaml::dump($data, $inline);

Yaml:dump second parameter is integer with default value 2 - "The level where you switch to inline YAML".

So, it can't be simplified neither like this nor

Yaml::dump($data, isset($options['inline']) ? (int)$options['inline'] : 2);

because default value for argument can be changed.

oops. sorry :) argument name is misleading.

aitboudad · 2015-05-13T16:17:31Z

I got some conflicts when I attempt to merge into 2.8, @gepo would you mind to fix the conflicts and open new PR for 2.8 branch to make it easy for me :) ?

jakzal · 2015-05-13T16:20:30Z

@aitboudad @gepo a rebase would probably work just fine, no need for a new PR. You'll then be able to switch the target branch while merging.

stof · 2015-05-13T17:35:10Z

@jakzal no we can't. After the rebase, the PR would include all 2.8 commits in the diff too, which means that it will totally mess the merging with GH.
The branch switching works fine when we rebase during the switching branch, not when the rebase is done previously without changing the target branch.

stof · 2015-05-13T17:36:28Z

src/Symfony/Component/Translation/Dumper/FileDumper.php

        }
    }

    /**
     * Transforms a domain of a message catalogue to its string representation.
+     * 
+     * This function is provided for backward compatibility.


this is not a BC layer, given that it is the one child class should implement now.

change it to

* Override this function in child class if $options is used for message formatting.

it it more correct?

jakzal · 2015-05-13T17:42:01Z

@stof I didn't mean rebasing to 2.8. It needs rebasing to 2.7. Can be
merged to 2.8.
On Wed, 13 May 2015 at 18:35, Christophe Coevoet notifications@github.com
wrote:

@jakzal https://github.com/jakzal no we can't. After the rebase, the PR
would include all 2.8 commits in the diff too, which means that it will
totally mess the merging with GH.
The branch switching works fine when we rebase during the switching
branch, not when the rebase is done previously without changing the target
branch.

—
Reply to this email directly or view it on GitHub
#14434 (comment).

…mp messages as tree instead of simple list. Dump messages as tree based on '.' character as a delimeter in path. For example this rray('foo.bar' => 'value') will be converted to array('foo' => array('bar' => 'value')). Correctly process cases like this ['foo.bar' => 'test1', 'foo' => 'test2'].

gepo · 2015-05-13T21:20:18Z

create #14630 for 2.8 branch

aitboudad · 2015-05-13T21:41:43Z

@jakzal it work only if there is not conflict.

aitboudad · 2015-05-13T21:42:23Z

closed in favor of #14630.

…d of simple list (gepo) This PR was merged into the 2.8 branch. Discussion ---------- [Translator] Dump translation constants as tree instead of simple list | Q | A | ------------- | --- | Bug fix? | no | New feature? | yes | BC breaks? | no | Deprecations? | no | Tests pass? | no | Fixed tickets | | License | MIT | Doc PR | PR #14434 for 2.8 branch Commits ------- 29ec5ca [Translation] add options 'as_tree', 'inline' to YamlFileDumper to dump messages as tree instead of simple list.

gepo force-pushed the yaml_tree_dumper branch from 305107d to 98f6407 Compare April 21, 2015 21:44

stof reviewed Apr 21, 2015
View reviewed changes

aitboudad added the Translation label Apr 22, 2015

stloyd reviewed Apr 22, 2015
View reviewed changes

stof reviewed Apr 22, 2015
View reviewed changes

stloyd reviewed Apr 23, 2015
View reviewed changes

gepo force-pushed the yaml_tree_dumper branch from 5854add to 0beb817 Compare April 25, 2015 09:33

aitboudad reviewed Apr 25, 2015
View reviewed changes

gepo force-pushed the yaml_tree_dumper branch from 0beb817 to 3e3f6e2 Compare April 26, 2015 10:11

aitboudad reviewed Apr 26, 2015
View reviewed changes

gepo force-pushed the yaml_tree_dumper branch 3 times, most recently from ab97816 to f9a6412 Compare April 27, 2015 09:11

gepo force-pushed the yaml_tree_dumper branch from f9a6412 to 8c1e21e Compare April 27, 2015 09:13

xabbuh reviewed Apr 28, 2015
View reviewed changes

gepo force-pushed the yaml_tree_dumper branch from 8c1e21e to 3c9bca7 Compare April 28, 2015 21:20

aitboudad added the Ready label May 4, 2015

xabbuh reviewed May 7, 2015
View reviewed changes

gepo force-pushed the yaml_tree_dumper branch from 3c9bca7 to 8f3f744 Compare May 7, 2015 18:53

jakzal reviewed May 13, 2015
View reviewed changes

stof reviewed May 13, 2015
View reviewed changes

gepo force-pushed the yaml_tree_dumper branch from 8f3f744 to 1fce98e Compare May 13, 2015 21:17

gepo mentioned this pull request May 13, 2015

[Translator] Dump translation constants as tree instead of simple list #14630

Merged

aitboudad closed this May 13, 2015

		@@ -9,6 +9,7 @@ To get the diff between two versions, go to https://github.com/symfony/symfony/c

		* 2.7.0-BETA1 (2015-04-10)

		* feature #14434 [Translation] add options 'as_tree', 'inline' to YamlFileDumper (gepo)

Uh oh!

[Translator] Dump translation constants as tree instead of simple list #14434

[Translator] Dump translation constants as tree instead of simple list #14434

Uh oh!

Conversation

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!