[#163] Added support for translation extractors for JS #238

jeremy-richard · 2018-01-29T14:39:08Z

Improve PR #164

Ping @willdurand, @JANorman, @monteiro @stof and @jcchavezs (thank you for helping me)

…Coffeescript and the inclusion of phpunit for the tests.

…urces/config/services.xml.

…he extractors stateless.

cf. https://github.com/willdurand/BazingaJsTranslationBundle/pull/164/files/413c12d0a87a7da345a61ed621c2099260d9fa7f#diff-6cb71bac1e0b1d9b34177249d6f1e0db willdurand#164 (comment)

jcchavezs · 2018-01-29T14:42:01Z

Great work @jeremy-richard!

jeremy-richard · 2018-01-29T14:42:49Z

I know CoffeeExtractorTest did not change from the previous PR #164

Is JsExtractorTest OK? Should I do the same modifications to CoffeeExtractorTest, or define dataProviders onto a new parent class aka BaseExtractorTest ?

jcchavezs · 2018-01-29T17:06:40Z

I'd recommend to do the abstract class as they behave the same.

Use an abstract class for the tests

jeremy-richard · 2018-01-31T09:18:31Z

Done, hopefully this feature will be available soon.

monteiro · 2018-02-10T09:09:15Z

@jeremy-richard looks good! Thanks for the great work.

Can you add just the missing break in all js and coffee files. That’s would be great.

Sorry for taking a lot of time.

willdurand · 2018-02-10T11:24:43Z

Do we really need two extractors? Why not allowing users to define the file extensions themselves? I can imagine .ts (typescript) as a useful extension. Thanks for this PR!

jeremy-richard · 2018-02-12T08:39:23Z

@monteiro yep, I'll do that.

@willdurand hi, it sounds like a good idea. How would you like to proceed?
I'd like to have a FrontEndExtractor, then set the extension to handle via the key bazinga_js_translation.frontend_extensions with the default value being ['js', 'jsx', 'coffee', 'ts']

Does the sequence will be the same for typescript then? (I've never code with typescript to be honest)

More flexible way to handle extraction from other sources than twig or php

jeremy-richard · 2018-02-14T13:20:04Z

@willdurand Hi, I found the way to have only one extractor to handle extraction from frontend files.
Let me know how to improve it.

stof · 2018-02-14T13:55:21Z

DependencyInjection/Configuration.php

@@ -34,6 +34,17 @@ public function getConfigTreeBuilder()
                    ->prototype('scalar')
                    ->end()
                ->end()
+                ->arrayNode('frontend')


I would rather name it extractor

stof · 2018-02-14T13:55:30Z

DependencyInjection/Configuration.php

@@ -34,6 +34,17 @@ public function getConfigTreeBuilder()
                    ->prototype('scalar')
                    ->end()
                ->end()
+                ->arrayNode('frontend')
+                  ->prototype('array')
+                    ->children()


indentation should use 4 spaces

stof · 2018-02-14T13:56:52Z

DependencyInjection/Configuration.php

@@ -34,6 +34,17 @@ public function getConfigTreeBuilder()
                    ->prototype('scalar')
                    ->end()
                ->end()
+                ->arrayNode('frontend')
+                  ->prototype('array')


do we actually need an array of configurations here ?

Based on your comment, no. We can use differents services.

stof · 2018-02-14T13:59:18Z

Resources/config/services.xml

@@ -26,5 +26,10 @@
            <argument>%kernel.root_dir%</argument>
            <tag name="console.command" command="bazinga:js-translation:dump" />
        </service>
+        <service id="bazinga.jstranslation.translation_frontend_extractor" class="Bazinga\Bundle\JsTranslationBundle\Extractor\FrontendExtractor" public="true">


should not be public. Extractors are totally fine as private services as Symfony does not retrieve them dynamically from the container. This would allow inlining the service.

stof · 2018-02-14T13:59:44Z

Resources/doc/index.md

+BazingaJsTranslationBundle includes two **extractors** for assets:
+
+- JsExtractor
+- CoffeeExtractors


this doc is now outdated. Please update it

Done for this part. I'll add how to use the new extractor when all the code will be good to merge.

stof · 2018-02-14T14:09:05Z

Extractor/FrontendExtractor.php

+
+final class FrontendExtractor extends AbstractFileExtractor implements ExtractorInterface
+{
+    const FUNCTION_STRING_ARGUMENT_REGEX = '\s?[\'"]([^"\'),]+)[\'"]\s?';


Spaces between the function call and the string should allow multiple spaces too, not just a single one.

stof · 2018-02-14T14:09:22Z

Tests/Extractor/FrontendExtractorTest.php

+    /**
+     * @var FrontendExtractor
+     */
+    protected $extractor;


should be private

stof · 2018-02-14T14:10:55Z

Tests/Extractor/FrontendExtractorTest.php

+    public function setUp()
+    {
+      $container = $this->getContainer();
+      $this->extractor = $container->get('bazinga.jstranslation.translation_frontend_extractor');


OK, this is the only place where you need the service to be public. But there is a much simpler way to handle all this: don't use a WebTestCase to test the FrontendExtractor but instantiate it directly in the test (the only other service it depends on is the Filesystem, which is dead easy to instantiate)

stof · 2018-02-14T14:11:38Z

Tests/Extractor/FrontendExtractorTest.php

+    }
+
+    /**
+     * @dataProvider resourcesWithATransChoiceFunctionUsage


no need to create data providers in case you have a single resource to process. Put the name of the file directly in the test.

stof · 2018-02-14T14:12:41Z

Extractor/FrontendExtractor.php

+        $messages = $this->getMessagesForSequence($fileContent, $sequence);
+
+        foreach ($messages as $message) {
+            $catalogue->set($message, $this->prefix.$message);


Setting them always in the messages domain is wrong. The best solution would be to support extracting the domain properly (but this is hard). But the basic solution is at least to respect the default domain being configured (which may be changed to something else than messages which is very handy when your frontend needs less than 1% of all your translation messages)

Then should we specify the domain in the constructor? (as the TranslationDumper does)
You said we can use the default domain, do you know the way to retrieve it frome this part of the code?

The default domain should indeed be injected in the constructor.

stof · 2018-02-14T15:04:50Z

Extractor/FrontendExtractor.php

+    {
+        $argumentsRegex = self::REGEX_DELIMITER
+            .$this->sequence
+            .self::FUNCTION_STRING_ARGUMENT_REGEX


why making the part about the function call configurable, but hardcoding the string argument regex ? Why not making the whole thing configurable instead ? Different cases might have different regexes for string arguments (thus, your own regex about string argument may not be the best one, as it forbids having any quotes inside the string, even when it is not the delimiting quote)

I don't really know. I forked this feature where @jcchavezs left it.

I would say that when I first wrote this I saw it convenient to make the keywords trans configurable but now I think it is not a problem so I would hardcode all. I don't think having everything configurable is a valid use case.

stof · 2018-02-14T15:05:34Z

Resources/config/services.xml

+            <argument type="service" id="filesystem"></argument>
+            <tag name="translation.extractor" alias="frontend" />
+            <argument></argument> <!-- extensions -->
+            <argument></argument> <!-- sequence -->


you don't have anything configuring this service anymore in the DI extension

stof · 2018-02-14T15:06:38Z

DependencyInjection/Configuration.php

@@ -34,6 +34,14 @@ public function getConfigTreeBuilder()
                    ->prototype('scalar')
                    ->end()
                ->end()
+                ->arrayNode('extractor')
+                    ->prototype('array')


as you define a prototyped node for extractors, this still requires configuring extractors explicitly, otherwise you would get none. I would rather see a config like that:

bazinga_js_translation: extraction: js: true # pre-configured extractor for JS, JSX and TS) coffee_script: false # separate service from JS with a different regex suited for the Coffee syntax as it may not have braces) custom_extractors: - { extensions: 'crazy', sequence: '->trans\(' }

This way, we can define a few built-in extractors that users just enable or disable, without having to care about what the regex is (and if they want a different config, they disable the built-in service and register a custom one)

I'm kind of lost here. How can I enable/disable dynamically a service from being instanciate by this config?
Or should I use this config to define the extractors in my own services configuration and delete the default one on this bundle?

delete the default one on this bundle

which default one ?

The one with the id bazinga.jstranslation.translation_frontend_extractor from the Resources/config/services.xml

all this configuration here is about defining some bazinga.jstranslation.* services (which is missing currently, as you don't process the config anymore).

The issue with your existing configuration tree is that you only have a config corresponding to my custom_extractors proposal, i.e. extractors configured in userland. So by default, there would be no extractor at all if they don't configure anything.

My proposed configuration would allow the bundle to manage itself the extensions and regex for the case of JS and CoffeeScript (and whatever pre-configured extractor we want to expose), while still allowing users to define more of them (I'm not even sure that second part is necessary, as they could just define some services themselves).

We must have some extractor service defined in the bundle. But we must have them working out of the box for users.

I agree. More commits to come to fix this feature.

stof · 2018-02-14T15:07:11Z

DependencyInjection/Configuration.php

+                ->arrayNode('extractor')
+                    ->prototype('array')
+                        ->children()
+                            ->scalarNode('extensions')->defaultValue(array('js', 'jsx', 'ts'))->end()


this is not a scalar node. This is a prototyped array node. Otherwise, users won't be able to configure multiple values.

stof · 2018-02-14T15:43:37Z

DependencyInjection/Configuration.php

@@ -34,6 +34,14 @@ public function getConfigTreeBuilder()
                    ->prototype('scalar')
                    ->end()
                ->end()
+                ->arrayNode('extractor')


this config is currently dead code, as you remove all usage of this config from the DI extension

jcchavezs and others added 10 commits January 29, 2018 11:31

[willdurand#163] Added support for translation extractors for JS and …

6b4463a

…Coffeescript and the inclusion of phpunit for the tests.

[willdurand#163] Fixed a bug in the declaration of extractors in Reso…

3743264

…urces/config/services.xml.

[willdurand#163] Added documentation for the extractors.

7a0bc62

[willdurand#163] Fixed a bug when using the finder in order to make t…

7f3b2ab

…he extractors stateless.

[willdurand#163] Fixes based on PR comments.

9f3d600

[willdurand#163] Fixed a bug in the regex for extracting transChoice.

32f738c

[willdurand#163] Fixed compatibility with PHP 5.3.

5aaac48

[willdurand#163] Fixes based on PR comment

4137824

cf. willdurand#164 (comment)

[willdurand#163] Fixes JsExtractorTest based on PR comment

4b1f6c4

cf. https://github.com/willdurand/BazingaJsTranslationBundle/pull/164/files/413c12d0a87a7da345a61ed621c2099260d9fa7f#diff-6cb71bac1e0b1d9b34177249d6f1e0db willdurand#164 (comment)

[willdurand#163] Fixes services as Finder isn't one

d57b413

willdurand self-requested a review January 29, 2018 14:39

[willdurand#163] Update CoffeeExtractorTest and JsExtractorTest

e33a819

[willdurand#163] Fixes based on PR comments.

f243f89

Use an abstract class for the tests

jeremy-richard added 2 commits February 12, 2018 09:53

Add missing break in all js and coffee files

fb372e7

Here Comes a New Challenger : FrontendExtractor

0ee29c6

More flexible way to handle extraction from other sources than twig or php

Fix Call to undefined method ArrayNodeDefinition::arrayPrototype()

2b2c5d4

stof suggested changes Feb 14, 2018

View reviewed changes

Fixes based on PR comments

e6dba39

jeremy-richard force-pushed the 163_addition_of_js_and_coffee_extractor branch from 98494a1 to e6dba39 Compare February 14, 2018 15:05

stof suggested changes Feb 14, 2018

View reviewed changes

stof reviewed Feb 14, 2018

View reviewed changes

[#163] Added support for translation extractors for JS #238

Are you sure you want to change the base?

[#163] Added support for translation extractors for JS #238

Conversation

jeremy-richard commented Jan 29, 2018

jcchavezs commented Jan 29, 2018

jeremy-richard commented Jan 29, 2018

jcchavezs commented Jan 29, 2018

jeremy-richard commented Jan 31, 2018

monteiro commented Feb 10, 2018

willdurand commented Feb 10, 2018

jeremy-richard commented Feb 12, 2018

jeremy-richard commented Feb 14, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment