Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[Translator] Dump translation constants as tree instead of simple list#14434
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
methods should be protected (there is no reason to make them extension points yet)
stof commentedApr 21, 2015
I think this should be configurable through an option |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Should be:
$elem[$part] =array();
aitboudad commentedApr 22, 2015
This look nice addition, what do you think to move this feature into `yaml dumper' ? |
jakzal commentedApr 22, 2015
@aitboudad I wouldn't add this feature to the yaml component as it's not part of the yaml standard. |
gepo commentedApr 22, 2015
ok, sure, did it |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This is not how it should work. It should rely on options passed to the dump method (see the interface)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I'd prefer2. it's not a BC break but it must be documented in the UPGRADE file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
protectedfunction format(MessageCatalogue$messages,$domain,$options =array());
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
with a typehint on the options argument
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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 commentedApr 22, 2015
Also add this behavior to json and php dumpers. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
if (isset($options['as_tree'])) {}
gepo commentedApr 25, 2015
squash all commits into one to easy review. move expandToTree to separate class and use it only in YamlFileDumper |
CHANGELOG-2.7.md Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
you should updateCHANGELOG of Translation and the target should be2.8
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This function is already documented inFileDumper, so useinheritdoc
/** * {@inheritdoc}*/ab97816 tof9a6412Compareaitboudad commentedApr 27, 2015
👍, ping @symfony/deciders |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
array should bestring[]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Should be above the YAML componentuse statement.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
It was fixed
xabbuh commentedMay 12, 2015
👍 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
this could be simplified:
$inline =isset($options['inline']) &&$options['inline'] >0;return Yaml::dump($data,$inline);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
oops. sorry :) argument name is misleading.
aitboudad commentedMay 13, 2015
I got some conflicts when I attempt to merge into |
jakzal commentedMay 13, 2015
@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 commentedMay 13, 2015
@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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
this is not a BC layer, given that it is the one child class should implement now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
change it to
* Override this function in child class if $options is used for message formatting.it it more correct?
jakzal commentedMay 13, 2015
@stof I didn't mean rebasing to 2.8. It needs rebasing to 2.7. Can be
|
…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 commentedMay 13, 2015
create#14630 for 2.8 branch |
aitboudad commentedMay 13, 2015
@jakzal it work only if there is not conflict. |
aitboudad commentedMay 13, 2015
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 branchCommits-------29ec5ca [Translation] add options 'as_tree', 'inline' to YamlFileDumper to dump messages as tree instead of simple list.
Dump translations constants as tree based on '.' character as a delimeter in path.
YamlFileDumper converts constants like these:
to next .yml file:
This PR convert the same input to this:
Which is more useful if you use translation import/export tools while storing translations in YAML files.