[2.1] Extracting translation messages from templates #2051
Conversation
| return null; | ||
| } | ||
| // is a trans filter | ||
| if($node->getNode('filter')->getAttribute('value') == 'trans') { |
There was a problem hiding this comment.
there is a missing space after if. And you should use ===
|
@michelsalib Could you try to refactor the code to make it more flexible by moving the creating of the file to the dumpers to support other outputs (database...) ? |
|
You are right, I shall do it tonight. |
| * file that was distributed with this source code. | ||
| */ | ||
|
|
||
| namespace Symfony\Bundle\TwigBundle\Translation; |
There was a problem hiding this comment.
what about moving this class in the bridge namespace as it depends only on the components, the bridge and Twig ?
It would make it possible to reuse it more easily outside Symfony, for instance in Silex.
There was a problem hiding this comment.
It is indeed possible. I will include it in the coming refactoring.
|
I just pushed a refactoring that should allow more flexibility. Dumpers are now responsive for writing the files. This way it is now possible to implement the DumperInterface that dump to a database and add it to the TranslationWriter. |
| public function writeTranslations(MessageCatalogue $catalogue, $format, $options = array()) | ||
| { | ||
| // get the right dumper | ||
| $dumper = $this->dumpers[$format]; |
There was a problem hiding this comment.
you should first check if there is a dumper for the format and throw an exception otherwise.
Commits ------- dea46c7 [Translation] support CsvDumper [Translation] support CsvDumper [Translation] support CsvDumper [Translation] support CsvDumper [Translation] support CsvDumper Discussion ---------- [2.1][Translation] support CsvDumper --------------------------------------------------------------------------- by michelsalib at 2011/09/05 04:56:47 -0700 The DumperInterface is probably going to change soon, it should be considered when merging : #2051.
|
To be consistent with other dumpers in the framework, the dumpers extending |
| 'force', null, InputOption::VALUE_NONE, | ||
| 'Should the update be done' | ||
| ) | ||
| )); |
There was a problem hiding this comment.
You should add a setHelp() method to describe how to use the command and the different arguments/options (especially the prefix as its usage is -- at least to me -- not obvious).
|
A general note on PHPDoc: The first line of a phpdoc ends with a dot and starts with a present verb like in |
|
I fixed most of the remarks. I just need to go through the phpdoc (in a few minutes). |
<
8000
p dir="auto">@michelsalib you should use git rebase (see docs) instead of git merge.
|
|
@stloyd sorry. I will rebase (squash) everything when I am finished. |
|
Hey, it might be a little bit late, but may i ask a not code-related question? Is it correct that the If so,
|
|
@stloyd I tried to do a |
|
@michelsalib for now just work as it is ;-) When I back from work I will try to help you to rebase it properly. |
|
@stloyd Thank you ! |
|
@michelsalib I was trying to rebase your code and revert it this bad commit (merge), but without success. IMO best way would be making brand new branch based on symfony/master, and the I'm sorry I can't give you more help... |
| /** | ||
| * {@inheritDoc} | ||
| */ | ||
| public function load($directory, MessageCatalogue $catalog) |
| public function extract($directory, MessageCatalogue $catalogue) | ||
| { | ||
| foreach ($this->extractors as $extractor) { | ||
| $extractor->load($directory, $catalogue); |
|
I just squashed and did a last polish of the code. You might want to read it again before merge. |
|
ok, code looks really good now. I think the only missing thing is some more unit tests (for the extractors for instance). |
|
Thanks, I'll look into it tomorrow. |
| <parameter key="translation.dumper.yml.class">Symfony\Component\Translation\Dumper\YamlDumper</parameter> | ||
| <parameter key="translation.dumper.qt.class">Symfony\Component\Translation\Dumper\QtTranslationsDumper</parameter> | ||
| <parameter key="translation.dumper.csv.class">Symfony\Component\Translation\Dumper\CsvDumper</parameter> | ||
| <parameter key="translation.loader.csv.class">Symfony\Component\Translation\Loader\CvsFileLoader</parameter> |
There was a problem hiding this comment.
Typo here: should be CsvFileLoader. Fabien corrected it in a recent commit, you must have missed it :)
There was a problem hiding this comment.
You are right. I probably got it wrong when rebuilding by hand the whole PR earlier today.
I'll fix it tomorrow while adding unit tests.
|
Hi, |
|
As the extractor is in bridge, the tests should be in the folder containing the tests for the bridge in tests/ |
|
Thanks @stof, it is now fixed. |
| */ | ||
| private $twig; | ||
|
|
||
| public function __construct(Twig_Environment $twig) |
There was a problem hiding this comment.
Below you use direct call to \Twig_Node, so here IMO you should use same ;-)
|
thanks @stloyd, it is fixed now. |
-- add missing files -- tweak translation command files -- dumpers are now responsive for writting the files -- moved the twig extractor the bridge -- clear temp files after unit tests -- check the presence of dumper in translation writer -- General cleaning of the code -- clean phpDoc -- fix PHPDoc -- fixing class name in configuration -- add unit tests for extractors (php and twig) -- moved test to correct location -- polish the code -- polish the code
|
Fixed again ;) |
Commits ------- ef322f6 -- add command that extracts translation messages from templates Discussion ---------- [2.1] Extracting translation messages from templates As seen here #1283 and here #2045, I push the command that extract translation from templates. There are still a lot of new things here, but it seems more manageable. --------------------------------------------------------------------------- by stof at 2011/09/04 02:04:40 -0700 @michelsalib Could you try to refactor the code to make it more flexible by moving the creating of the file to the dumpers to support other outputs (database...) ? --------------------------------------------------------------------------- by michelsalib at 2011/09/04 02:35:50 -0700 You are right, I shall do it tonight. --------------------------------------------------------------------------- by michelsalib at 2011/09/04 11:53:35 -0700 I just pushed a refactoring that should allow more flexibility. Dumpers are now responsive for writing the files. This way it is now possible to implement the DumperInterface that dump to a database and add it to the TranslationWriter. I updated the tests accordingly. --------------------------------------------------------------------------- by fabpot at 2011/09/05 23:27:27 -0700 To be consistent with other dumpers in the framework, the dumpers extending `FileDumper` should use `FileDumper` as a suffix like in `YmlFileDumper`. --------------------------------------------------------------------------- by fabpot at 2011/09/05 23:41:12 -0700 A general note on PHPDoc: The first line of a phpdoc ends with a dot and starts with a present verb like in `Extracts translation messages from template files.` --------------------------------------------------------------------------- by michelsalib at 2011/09/06 01:23:31 -0700 I fixed most of the remarks. I just need to go through the phpdoc (in a few minutes). --------------------------------------------------------------------------- by stloyd at 2011/09/06 01:28:55 -0700 @michelsalib you should use `git rebase` (see [docs](http://symfony.com/doc/current/contributing/code/patches.html#id1)) instead of `git merge`. --------------------------------------------------------------------------- by michelsalib at 2011/09/06 01:31:06 -0700 @stloyd sorry. I will rebase (squash) everything when I am finished. --------------------------------------------------------------------------- by kaiwa at 2011/09/06 01:31:18 -0700 Hey, it might be a little bit late, but may i ask a not code-related question? Is it correct that the `--force` option only means to write the output to a file instead of writing it to stdout? If so, 1. is it semantically correct? I mean... i'm not a native english speaker, but from unix programs i'm used to interpret a `--force` option as something like "overwrite", "ignore errors" or "suppress warnings". An option which is used in case of trouble most time. Feels confusing to me to be forced ;-) to use the `--force` option for simply writing to files. 2. does it makes sense to have a default behaviour instead of requiring the user to give either `--force` or `--dump-messages`? In which cases does the user wants to dump the messages to the console? Is it only for debugging / to review the messages? --------------------------------------------------------------------------- by michelsalib at 2011/09/06 01:33:58 -0700 @kaiwa Your concerns seems perfectly right. Initially I just wanted to mimic the `doctrine:schema:update` command. But it can be changed. @fabpot what do you think ? --------------------------------------------------------------------------- by michelsalib at 2011/09/06 02:01:22 -0700 @stloyd I tried to do a `git rebase` and I am quite lost. I think I messed something when merge instead of rebase. How can I clean/squash my PR properly ? --------------------------------------------------------------------------- by stloyd at 2011/09/06 02:11:29 -0700 @michelsalib for now just work as it is ;-) When I back from work I will try to help you to rebase it properly. --------------------------------------------------------------------------- by michelsalib at 2011/09/06 02:12:17 -0700 @stloyd Thank you ! --------------------------------------------------------------------------- by stloyd at 2011/09/06 12:39:18 -0700 @michelsalib I was trying to rebase your code and revert it this _bad_ commit (merge), but without success. IMO best way would be making _brand new_ branch based on symfony/master, and the `git cherry-pick` ([hint](http://ariejan.net/2010/06/10/cherry-picking-specific-commits-from-another-branch)) commits you need (almost all in this PR but without this one with merge), then you will need to apply again _by hand_ changes from commit michelsalib@fce24c7 (this clean up you have done there). I'm sorry I can't give you more help... --------------------------------------------------------------------------- by michelsalib at 2011/09/07 09:41:36 -0700 @stloyd I finally succeed to fix this PR. Thanks to your help! I must admit the whole thing with `cherry-pick` command was quite epic. @fabpot Don't be sorry, I am about to fix the PHPDoc. I shall squash right after. --------------------------------------------------------------------------- by michelsalib at 2011/09/07 10:07:20 -0700 I just squashed and did a last polish of the code. You might want to read it again before merge. --------------------------------------------------------------------------- by fabpot at 2011/09/07 11:40:48 -0700 ok, code looks really good now. I think the only missing thing is some more unit tests (for the extractors for instance). --------------------------------------------------------------------------- by michelsalib at 2011/09/07 12:18:59 -0700 Thanks, I'll look into it tomorrow. --------------------------------------------------------------------------- by michelsalib at 2011/09/08 15:13:10 -0700 Hi, I just added unit tests for both extractors (php and yaml). Concerning the yaml extractor, the test is quite tricky because I need to mock the twig environment while returning a twig tree that contain a trans block and a trans filter. But it work fine with as little side effects as possible. Also I am not sure that the test should be in the TwigBundle. IMO, the PR seems ready. --------------------------------------------------------------------------- by stof at 2011/09/08 15:34:41 -0700 As the extractor is in bridge, the tests should be in the folder containing the tests for the bridge in tests/ --------------------------------------------------------------------------- by michelsalib at 2011/09/08 15:41:45 -0700 Thanks @stof, it is now fixed. --------------------------------------------------------------------------- by michelsalib at 2011/09/09 00:48:47 -0700 thanks @stloyd, it is fixed now. --------------------------------------------------------------------------- by michelsalib at 2011/09/09 01:25:24 -0700 Fixed again ;)
|
@michelsalib the DumperInterface has an $options param, but not the Dumpers ($domain) ? |
As seen here #1283 and here #2045, I push the command that extract translation from templates.
There are still a lot of new things here, but it seems more manageable.